Improve GC for deactivated client's nodes #926
Conversation
WalkthroughThe changes introduce a new method called Changes
Sequence Diagram(s)sequenceDiagram
participant A as Actor
participant V as VersionVector
participant T as TimeTicket
A->>V: Call afterOrEqual(actorId)
V->>V: Check if actorId exists
alt Actor ID found
V-->>A: Return boolean based on existing lamport
else Actor ID not found
V->>V: Call minLamport()
V-->>A: Return boolean based on minLamport and TimeTicket
end
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@hackerwins has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/sdk/test/integration/gc_test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/sdk".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/sdk/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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 using PR comments)
Other keywords and placeholders
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/document/time/version_vector.ts(2 hunks)
🔇 Additional comments (1)
packages/sdk/src/document/time/version_vector.ts (1)
Line range hint 61-117: Verify GC behavior with these changes
The implementation aligns with the PR objectives for improving garbage collection of nodes from deactivated clients. Let's verify the behavior:
Please ensure that:
- The GC logic properly uses these changes to clean up nodes from deactivated clients
- Test cases cover scenarios with both active and deactivated clients
- Edge cases are tested (empty vector, single client, all clients deactivated)
…nto add-min-lamport-to-run-gc-properly
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/sdk/test/integration/gc_test.ts (3)
1603-1698: Test case needs enhancement for better verification.The test case structure is good but could be improved in several ways:
- Add descriptive messages to assertions to clarify what's being tested:
- assert.equal(doc2.getGarbageLen(), 2); - assert.equal(doc2.getVersionVector().size(), 2); + assert.equal(doc2.getGarbageLen(), 2, 'Should have 2 garbage nodes after client1 deactivation'); + assert.equal(doc2.getVersionVector().size(), 2, 'Version vector should still contain both clients');
- Add verification of document content to ensure data integrity:
await client1.deactivate(); + assert.equal(doc2.getRoot().t.toString(), 'a', 'Document content should be "a" after deletions'); assert.equal(doc2.getGarbageLen(), 2);
1692-1694: TODO comment needs more context.The TODO comment references issue #1058 but doesn't explain the impact on this test. Consider adding more context about what behavior is expected once the issue is resolved.
Consider updating the comment to:
- // TODO(JOOHOJANG): remove below comments after https://github.com/yorkie-team/yorkie/issues/1058 resolved - // Due to https://github.com/yorkie-team/yorkie/issues/1058, removing deactivated client's version vector is not working properly now. + // TODO(JOOHOJANG): Enable these assertions after https://github.com/yorkie-team/yorkie/issues/1058 is resolved + // Currently, the deactivated client's version vector is not properly removed from the document + // Expected behavior: After client deactivation and sync, the version vector should only contain + // the active client, and garbage nodes should be collected
1695-1697: Consider adding test coverage for partial functionality.While the complete test is blocked by issue #1058, consider adding assertions for the functionality that does work correctly. This will help catch regressions in the working parts while waiting for the issue to be resolved.
- //await client2.sync(); - //assert.equal(doc2.getGarbageLen(), 0); - //assert.equal(doc2.getVersionVector().size(), 1); + // Verify current behavior while issue #1058 is pending + await client2.sync(); + assert.equal(doc2.getRoot().t.toString(), 'a', 'Document content should remain stable after sync'); + // TODO: Following assertions will be enabled after #1058 is resolved + // assert.equal(doc2.getGarbageLen(), 0, 'Garbage should be collected after sync'); + // assert.equal(doc2.getVersionVector().size(), 1, 'Version vector should only contain active client');
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/document/time/version_vector.ts (1)
Line range hint
61-118: Consider caching minLamport for performance optimization.Since
minLamportis used for garbage collection decisions and could be called frequently, consider:
- Caching the minimum value and updating it only when the vector changes
- Adding a method to check if a vector has changed since the last minLamport calculation
This would improve performance for garbage collection operations, especially in large documents with many deactivated clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/document/time/version_vector.ts(2 hunks)
🔇 Additional comments (1)
packages/sdk/src/document/time/version_vector.ts (1)
116-118: Previous review comment about logical inconsistency is still valid.
The logical inconsistency between the method name (afterOrEqual) and the implementation (using strict >) remains an issue.
|
I found some defection in min version vector algorithm while watching CI failure. To summarize briefly, there is a case where the min version vector is empty. When the last user detaches a document, all documents are in a detached state, so an empty min version vector is created at the time of generation. While pushpulling this detach change, we end up receiving an empty min version vector, and in trying to calculate minLamport from this empty min version vector, we get an int64Max value. Therefore gc runs and error occured. In my opinion, except for this specific case (where the last document detaches), this situation is unlikely to occur otherwise. So, I think adding an exception handling should resolve it. What do you think? @hackerwins |
Error occured because of split operation's defec. When applying remote split operation, tombstone-marked node doesn't carried by its parent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/sdk/test/integration/gc_test.ts (3)
1692-1693: Track the version vector removal issue.The TODO comment references issue #1058 regarding the improper removal of deactivated client's version vector. Consider adding a link to the issue for better tracking.
- // TODO(JOOHOJANG): remove below comments after https://github.com/yorkie-team/yorkie/issues/1058 resolved - // Due to https://github.com/yorkie-team/yorkie/issues/1058, removing deactivated client's version vector is not working properly now. + // TODO(JOOHOJANG): Remove these comments after https://github.com/yorkie-team/yorkie/issues/1058 is resolved. + // See: https://github.com/yorkie-team/yorkie/issues/1058 + // Issue: Removing deactivated client's version vector is not working properly.
1700-1702: Consider shortening the test name for better readability.The test name is quite long and could be more concise while maintaining clarity.
- it('attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document, but no tombstones)', async function ({ + it('should run gc when last client detaches (no tombstones)', async function ({
2034-2096: Improve documentation of skipped test case.The test case contains valuable information about error scenarios but could benefit from more structured documentation. Consider adding JSDoc comments to clearly document:
- The purpose of the test
- The error conditions being tested
- The expected behavior once fixed
Add JSDoc comments before the test:
/** * Tests error handling when GC runs with empty min version vector. * * Current issues: * 1. Empty min version vector when last client detaches * 2. Int64max value returned from VersionVector.minLamport() * 3. Premature removal of tombstones affects split operations * * @see Related issue: [Add issue link here] */ it.skip('attach > pushpull > detach lifecycle...')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/test/integration/gc_test.ts(1 hunks)
🔇 Additional comments (1)
packages/sdk/test/integration/gc_test.ts (1)
1603-2096: Well-structured test cases with good coverage.
The new test cases effectively cover important edge cases in the garbage collection logic, particularly around client deactivation and document lifecycle. The tests are well-organized with clear stages:
- Setup and client activation
- Document manipulation
- Synchronization
- Verification of GC behavior
The test assertions and version vector checks provide good validation of the system's behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
packages/sdk/test/integration/gc_test.ts (4)
1692-1694: Track the blocking issue for deactivated client's version vector.The TODO comment indicates a known issue (#1058) that prevents proper removal of the deactivated client's version vector. Consider adding a link to the issue in the comment for better tracking.
-// TODO(JOOHOJANG): remove below comments after https://github.com/yorkie-team/yorkie/issues/1058 resolved -// Due to https://github.com/yorkie-team/yorkie/issues/1058, removing deactivated client's version vector is not working properly now. +// TODO(JOOHOJANG): Uncomment assertions below after fixing https://github.com/yorkie-team/yorkie/issues/1058 +// Issue: Removing deactivated client's version vector is not working properly +// Tracking: https://github.com/yorkie-team/yorkie/issues/1058
1603-1698: Test case looks good but could be improved.The test case effectively verifies garbage collection behavior with deactivated clients. However, consider adding more assertions to verify the document state and content after client deactivation.
Consider adding these assertions:
await client1.deactivate(); assert.equal(doc2.getGarbageLen(), 2); assert.equal(doc2.getVersionVector().size(), 2); +assert.equal(doc2.getRoot().t.toString(), 'a'); // Verify final text content +assert.equal(doc1.getRoot().t.toString(), 'a'); // Verify both docs are in sync
1700-1700: Fix typo in test name.There's a typo in the test name: 'exsits' should be 'exists'.
- it('attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document, but no tombstone exsits)', async function ({ + it('attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document, but no tombstone exists)', async function ({
1700-1869: Test case is thorough but could be more maintainable.The test effectively verifies the garbage collection behavior during the document lifecycle. However, the test could be more maintainable by extracting repeated version vector assertions into a helper function.
Consider creating a helper function:
function assertVersionVector(doc: Document, expected: Array<{actor: string, lamport: bigint}>) { assert.equal( versionVectorHelper(doc.getVersionVector(), expected), true, ); }This would reduce code duplication and make the test more maintainable.
packages/sdk/test/integration/tree_test.ts (6)
Line range hint
2481-2488: Implement the skipped test 'contained-split-and-delete-the-whole-original-and-split-nodes'The test
contained-split-and-delete-the-whole-original-and-split-nodesis marked as skipped. Implementing this test is important to validate the behavior when both the original and split nodes are deleted after a split operation. This will help ensure the robustness of the split and delete functionalities under concurrent conditions.I'm happy to help implement this test case or debug any related issues.
Line range hint
2535-2540: Consider enabling the skipped test 'contained-merge-and-merge-at-the-same-level'The test
contained-merge-and-merge-at-the-same-levelis currently skipped. If this test is stable and passes, consider removing the.skipto include it in the test suite. Enabling this test will enhance coverage for merge operations occurring at the same level.
Line range hint
2557-2563: Ensure the test 'contained-merge-and-insert' is implemented and passingThe test
contained-merge-and-insertis skipped. Implementing this test will help verify the correctness of concurrent merge and insert operations, which is crucial for maintaining data integrity in concurrent editing scenarios.Let me know if you need assistance in implementing this test case.
Line range hint
2587-2593: Enable or remove the skipped test 'contained-merge-and-delete-contents-in-merged-node'The test
contained-merge-and-delete-contents-in-merged-nodeis currently skipped. If the test is ready and passes, please enable it to increase test coverage. If it's no longer needed, consider removing it to keep the test suite clean.
Line range hint
2610-2617: Implement or remove the skipped test 'side-by-side-merge-and-insert'The test
side-by-side-merge-and-insertis marked as skipped. Implementing this test will help ensure the correctness of concurrent side-by-side merge and insert operations, which is important for handling complex concurrent edits.
Line range hint
2635-2642: Consider implementing the skipped test 'side-by-side-merge-and-delete'The test
side-by-side-merge-and-deleteis currently skipped. Enabling this test after implementation will help validate the behavior of side-by-side merge and delete operations under concurrent scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/test/integration/gc_test.ts(1 hunks)packages/sdk/test/integration/tree_test.ts(1 hunks)
🔇 Additional comments (1)
packages/sdk/test/integration/tree_test.ts (1)
Line range hint 2678-2686: Review the need for the skipped test 'contained-split-and-delete-contents-in-split-node'
The test contained-split-and-delete-contents-in-split-node is skipped and includes a comment about a split operation defect causing a GC error. If this issue has been resolved, consider implementing the test. Otherwise, updating the comment with the current status would be helpful.
| // TODO(JOOHOJANG) | ||
| // split operation's defect cause GC error. | ||
| // remove this comment after implement split operation completely. | ||
| it.skip('contained-split-and-delete-contents-in-split-node', async function ({ |
There was a problem hiding this comment.
Address the TODO comment and implement the skipped test
The test contained-split-and-delete-contents-in-split-node is currently skipped and includes a TODO comment indicating that a defect in the split operation causes a GC error. To ensure comprehensive test coverage and verify the split operation's functionality, please implement this test case and remove the TODO comment once the issue is resolved.
Would you like assistance in addressing the split operation defect and implementing this test case?
hackerwins
force-pushed
the
add-min-lamport-to-run-gc-properly
branch
from
8d69b61 to
2846be1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/src/document/time/version_vector.ts(2 hunks)packages/sdk/test/integration/tree_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/test/integration/tree_test.ts
🔇 Additional comments (1)
packages/sdk/src/document/time/version_vector.ts (1)
Line range hint 61-119: Consider distributed system implications
The changes to minLamport and afterOrEqual affect garbage collection behavior in a distributed setting. Please ensure:
- The new behavior maintains causal consistency across the distributed system
- Edge cases like network partitions and concurrent modifications are handled correctly
- Consider adding logging for debugging GC-related issues in production
- Document the GC behavior changes in the system architecture documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/sdk/test/integration/gc_test.ts (1)
1697-1697: Fix typo in test nameThere's a typo in the test name: "exsits" should be "exists"
- it('attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document, but no tombstone exsits)', async function ({ + it('attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document, but no tombstone exists)', async function ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/test/integration/gc_test.ts(1 hunks)
🔇 Additional comments (1)
packages/sdk/test/integration/gc_test.ts (1)
1603-2030: Well-structured test cases with comprehensive coverage
The new test cases effectively cover important garbage collection scenarios:
- Garbage collection behavior with deactivated clients
- Version vector lifecycle during document attachment/detachment
The tests are well-structured with clear setup, actions, and assertions. They help ensure the reliability of the garbage collection mechanism.
hackerwins
changed the title
What this PR does / why we need it?
If a node created by a deactivated client remains in the document, it means that the garbage collection did not run. The existing GC logic compares the Lamport of the node's ticket with the min version vector based on actor information. However, the min version vector does not contain information about the deactivated client.
To handle this case, when the actor information in the ticket is missing in the min version vector, we retrieve the minimum value from the min version vector. If this minimum value is greater than the Lamport in the ticket, the node is removed.
The reason is that a higher minimum value in the min version vector implies that all users have received that change, indicating it is safe to delete.
+)
I updated garbage collection design document includes this PR's information.
If you want to know, please read it.yorkie-team/yorkie#1061
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests