Update updated_at only when there are operations in changes
#935
Conversation
|
|
WalkthroughThe changes introduce enhanced logic for updating the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
Client->>Database: Update Document Presence
Database->>Database: Check Operations
alt No Operations
Database-->>Client: UpdatedAt remains unchanged
else Operations present
Database-->>Database: Update UpdatedAt to now
Database-->>Client: UpdatedAt modified
end
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
server/backend/database/testcases/testcases.go (1)
923-923: Add a descriptive comment for the test case.Consider adding a comment to describe the purpose of this test case for better readability.
// This test case verifies that the `UpdatedAt` timestamp is only updated when there are multiple operations in the change list.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/backend/database/memory/database.go (1 hunks)
- server/backend/database/mongo/client.go (1 hunks)
- server/backend/database/testcases/testcases.go (1 hunks)
Additional comments not posted (2)
server/backend/database/memory/database.go (1)
903-909: LGTM! But verify the function usage in the codebase.The code changes are approved. The logic ensures that the
UpdatedAtfield is updated only when there are actual operations, which aligns with the PR objectives.However, ensure that all function calls to
CreateChangeInfosare consistent with the new logic.Verification successful
LGTM! The function usage is consistent with the new logic.
The code changes ensure that the
UpdatedAtfield is updated only when there are actual operations, and this logic is correctly implemented across the codebase.
server/packs/packs.goserver/backend/database/database.goserver/backend/database/memory/database.goserver/backend/database/mongo/client.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateChangeInfos` are consistent with the new logic. # Test: Search for the function usage. Expect: Ensure correct implementation and consistency. rg --type go -A 5 $'CreateChangeInfos'Length of output: 8584
Script:
#!/bin/bash # Description: Gather context around the usage of `CreateChangeInfos` to ensure correct implementation and consistency. # Search for the function usage in the implementation files to gather more context. rg --type go -A 10 -B 10 'CreateChangeInfos' server/packs/packs.go rg --type go -A 10 -B 10 'CreateChangeInfos' server/backend/database/database.go rg --type go -A 10 -B 10 'CreateChangeInfos' server/backend/database/memory/database.go rg --type go -A 10 -B 10 'CreateChangeInfos' server/backend/database/mongo/client.goLength of output: 2389
server/backend/database/mongo/client.go (1)
895-901: LGTM! But verify the function usage in the codebase.The code changes are approved. The logic ensures that the
updated_atfield is updated only when there are actual operations, which aligns with the PR objectives.However, ensure that all function calls to
CreateChangeInfosare consistent with the new logic.
| ctx := context.Background() | ||
| docKey := helper.TestDocKey(t) | ||
|
|
||
| // 01. Create a client and a document then attach the document to the client. | ||
| clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name()) | ||
| docInfo1, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | ||
| docRefKey := docInfo1.RefKey() | ||
| assert.NoError(t, clientInfo.AttachDocument(docRefKey.DocID, false)) | ||
| assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo1)) | ||
|
|
||
| bytesID, _ := clientInfo.ID.Bytes() | ||
| actorID, _ := time.ActorIDFromBytes(bytesID) | ||
| doc := document.New(key.Key(t.Name())) | ||
| doc.SetActor(actorID) | ||
|
|
||
| // 02. update document only presence | ||
| assert.NoError(t, doc.Update(func(root *json.Object, p *presence.Presence) error { | ||
| p.Set("key", "val") | ||
| return nil | ||
| })) | ||
| pack := doc.CreateChangePack() | ||
| updatedAt := docInfo1.UpdatedAt | ||
| assert.NoError(t, db.CreateChangeInfos(ctx, projectID, docInfo1, 0, pack.Changes, false)) | ||
| docInfo2, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | ||
| assert.Equal(t, updatedAt, docInfo2.UpdatedAt) |
There was a problem hiding this comment.
Ensure proper error handling.
The code lacks error handling for several operations. Ensure to check for errors after each operation to prevent potential issues.
- docInfo1, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
+ docInfo1, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
+ assert.NoError(t, err)
- clientInfo.AttachDocument(docRefKey.DocID, false)
+ assert.NoError(t, clientInfo.AttachDocument(docRefKey.DocID, false))
- db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo1)
+ assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo1))
- bytesID, _ := clientInfo.ID.Bytes()
+ bytesID, err := clientInfo.ID.Bytes()
+ assert.NoError(t, err)
- actorID, _ := time.ActorIDFromBytes(bytesID)
+ actorID, err := time.ActorIDFromBytes(bytesID)
+ assert.NoError(t, err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx := context.Background() | |
| docKey := helper.TestDocKey(t) | |
| // 01. Create a client and a document then attach the document to the client. | |
| clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name()) | |
| docInfo1, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | |
| docRefKey := docInfo1.RefKey() | |
| assert.NoError(t, clientInfo.AttachDocument(docRefKey.DocID, false)) | |
| assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo1)) | |
| bytesID, _ := clientInfo.ID.Bytes() | |
| actorID, _ := time.ActorIDFromBytes(bytesID) | |
| doc := document.New(key.Key(t.Name())) | |
| doc.SetActor(actorID) | |
| // 02. update document only presence | |
| assert.NoError(t, doc.Update(func(root *json.Object, p *presence.Presence) error { | |
| p.Set("key", "val") | |
| return nil | |
| })) | |
| pack := doc.CreateChangePack() | |
| updatedAt := docInfo1.UpdatedAt | |
| assert.NoError(t, db.CreateChangeInfos(ctx, projectID, docInfo1, 0, pack.Changes, false)) | |
| docInfo2, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | |
| assert.Equal(t, updatedAt, docInfo2.UpdatedAt) | |
| ctx := context.Background() | |
| docKey := helper.TestDocKey(t) | |
| // 01. Create a client and a document then attach the document to the client. | |
| clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name()) | |
| docInfo1, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | |
| assert.NoError(t, err) | |
| docRefKey := docInfo1.RefKey() | |
| assert.NoError(t, clientInfo.AttachDocument(docRefKey.DocID, false)) | |
| assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo1)) | |
| bytesID, err := clientInfo.ID.Bytes() | |
| assert.NoError(t, err) | |
| actorID, err := time.ActorIDFromBytes(bytesID) | |
| assert.NoError(t, err) | |
| doc := document.New(key.Key(t.Name())) | |
| doc.SetActor(actorID) | |
| // 02. update document only presence | |
| assert.NoError(t, doc.Update(func(root *json.Object, p *presence.Presence) error { | |
| p.Set("key", "val") | |
| return nil | |
| })) | |
| pack := doc.CreateChangePack() | |
| updatedAt := docInfo1.UpdatedAt | |
| assert.NoError(t, db.CreateChangeInfos(ctx, projectID, docInfo1, 0, pack.Changes, false)) | |
| docInfo2, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true) | |
| assert.Equal(t, updatedAt, docInfo2.UpdatedAt) |
hackerwins
changed the title
updated_at only when there are operations in changes
What this PR does / why we need it:
The current implementation updates the 'updated_at' field of a document even if the content itself remains unchanged. This happens because changes reported by the client may contain both "presence" and "operations." With the changes introduced in this commit, the 'updated_at' timestamp will only be updated when there is more than one operation in the change list.
Initially, I attempted to address this issue by avoiding database operations when the changes were limited to presence updates. However, this approach was unsuccessful for the following reasons:
Consequently, it is necessary to store all changes in the database, even if they do not modify the core content, to maintain consistency.
In the context of applications like a whiteboard, where numerous operations can occur simply from moving a mouse pointer, this issue becomes more pronounced. Such frequent minor updates lead to a high volume of changes that primarily involve presence, inflating the ClientSeq and triggering server snapshots, which add to the server's load.
Proposed Solution
Initially, I considered separating presence data from document updates and managing it through an in-memory database like Redis. This would involve removing presence data from the push-pull synchronization mechanism and introducing a new API dedicated to presence management. However, this approach would require extensive modifications to the existing codebase and protocols, affecting not just the current system but other SDKs as well.
Given the complexities involved, I recommend keeping the current push-pull mechanism unchanged for stability and introducing a new API dedicated to broadcasting user presence. This strategy is ideal for applications where excessive updates are driven primarily by presence changes. Developers can shift presence management from the standard update method to this new broadcast API.
This broadcast API is specifically designed to manage presence updates independently. It broadcasts changes to all document subscribers efficiently without modifying the sequence number (ClientSeq) or permanently storing the updates. This design ensures optimal performance by minimizing unnecessary data load and sequence adjustments, making it highly suitable for managing frequent, low-impact changes.
Which issue(s) this PR fixes:
Fixes #916
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
UpdatedAttimestamp, now only updating when there are actual operations present.Tests
UpdatedAttimestamp based on document updates and operations.Bug Fixes
UpdatedAtfield is updated, ensuring more accurate timestamp management.