[DTP-1024] GC tombstoned map entries for LiveMap and objects in the global pool #1937
Conversation
WalkthroughThe pull request introduces several changes across multiple files to enhance garbage collection functionality within the LiveObjects plugin. Key additions include a new Changes
Possibly related PRs
Suggested reviewers
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1937/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1937/features
github-actions
bot
temporarily deployed
to
staging/pull/1937/typedoc
VeskeR
changed the base branch from
integration/liveobjects
to
DTP-986/handle-tombstone-and-object-delete
VeskeR
force-pushed
the
DTP-986/handle-tombstone-and-object-delete
branch
from
f370f62 to
4ff3647
Compare
Resolves DTP-1024
VeskeR
force-pushed
the
DTP-1024/gc-tombstoned-entries-and-objects
branch
from
e99520c to
1a598e0
Compare
![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 and nitpick comments (2)
src/plugins/liveobjects/liveobject.ts (1)
190-190: Add documentation for the abstract method.Please add JSDoc documentation for the
onGCInterval()method to explain its purpose, when it's called, and what implementing classes should do.src/plugins/liveobjects/liveobjects.ts (1)
30-31: Enhance the documentation comment.The current comment "Used by tests" could be more descriptive. Consider adding more context about what aspects of the defaults are being tested, such as:
-// Used by tests +// Exposed for testing garbage collection configuration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/plugins/liveobjects/defaults.ts(1 hunks)src/plugins/liveobjects/livecounter.ts(1 hunks)src/plugins/liveobjects/livemap.ts(6 hunks)src/plugins/liveobjects/liveobject.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(2 hunks)src/plugins/liveobjects/liveobjectspool.ts(3 hunks)test/common/modules/private_api_recorder.js(4 hunks)test/realtime/live_objects.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/plugins/liveobjects/livecounter.ts
🔇 Additional comments (4)
src/plugins/liveobjects/liveobjectspool.ts (1)
22-26: Ensure compatibility of unref usage across environments
The use of this._gcInterval.unref?.(); effectively prevents the Node.js event loop from being blocked by the interval. Please verify that the optional chaining operator ?. and the unref method are compatible with all target environments, including browsers and older Node.js versions, as unref may not be available.
src/plugins/liveobjects/defaults.ts (1)
1-10: Definition of DEFAULTS constants is clear and appropriate
The introduction of the DEFAULTS constant with properly documented gcInterval and gcGracePeriod provides clarity and configurability for garbage collection timings.
src/plugins/liveobjects/liveobject.ts (1)
131-137: LGTM!
The tombstonedAt() getter method is well-implemented and correctly marked as internal.
test/common/modules/private_api_recorder.js (1)
19-21: LGTM!
The new private API identifiers are well-organized, follow the existing naming pattern, and correctly reflect the private APIs used for garbage collection functionality.
Also applies to: 78-78, 119-119, 136-137
| if (value.tombstone === true && Date.now() - value.tombstonedAt! >= DEFAULTS.gcGracePeriod) { | ||
| keysToDelete.push(key); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using non-null assertions on tombstonedAt
In the onGCInterval method, the code uses value.tombstonedAt! with a non-null assertion. To prevent potential runtime errors if tombstonedAt is undefined, consider adding a null check to ensure safety.
Apply this diff to add a null check:
-if (value.tombstone === true && Date.now() - value.tombstonedAt! >= DEFAULTS.gcGracePeriod) {
+if (value.tombstone === true && value.tombstonedAt !== undefined && Date.now() - value.tombstonedAt >= DEFAULTS.gcGracePeriod) {📝 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.
| if (value.tombstone === true && Date.now() - value.tombstonedAt! >= DEFAULTS.gcGracePeriod) { | |
| keysToDelete.push(key); | |
| if (value.tombstone === true && value.tombstonedAt !== undefined && Date.now() - value.tombstonedAt >= DEFAULTS.gcGracePeriod) { | |
| keysToDelete.push(key); |
| forScenarios(tombstonesGCScenarios, async function (helper, scenario) { | ||
| try { | ||
| helper.recordPrivateApi('write.LiveObjects._DEFAULTS.gcInterval'); | ||
| LiveObjectsPlugin.LiveObjects._DEFAULTS.gcInterval = 500; | ||
| helper.recordPrivateApi('write.LiveObjects._DEFAULTS.gcGracePeriod'); | ||
| LiveObjectsPlugin.LiveObjects._DEFAULTS.gcGracePeriod = 250; | ||
|
|
||
| const liveObjectsHelper = new LiveObjectsHelper(helper); | ||
| const client = RealtimeWithLiveObjects(helper); | ||
|
|
||
| await helper.monitorConnectionThenCloseAndFinish(async () => { | ||
| const channelName = scenario.description; | ||
| const channel = client.channels.get(channelName, channelOptionsWithLiveObjects()); | ||
| const liveObjects = channel.liveObjects; | ||
|
|
||
| await channel.attach(); | ||
| const root = await liveObjects.getRoot(); | ||
|
|
||
| // helper function to spy on the GC interval callback and wait for a specific number of GC cycles. | ||
| // returns a promise which will resolve when required number of cycles have happened. | ||
| const waitForGCCycles = (cycles) => { | ||
| const onGCIntervalOriginal = liveObjects._liveObjectsPool._onGCInterval; | ||
| let gcCalledTimes = 0; | ||
| return new Promise((resolve) => { | ||
| helper.recordPrivateApi('replace.LiveObjects._liveObjectsPool._onGCInterval'); | ||
| liveObjects._liveObjectsPool._onGCInterval = function () { | ||
| helper.recordPrivateApi('call.LiveObjects._liveObjectsPool._onGCInterval'); | ||
| onGCIntervalOriginal.call(this); | ||
|
|
||
| gcCalledTimes++; | ||
| if (gcCalledTimes >= cycles) { | ||
| resolve(); | ||
| liveObjects._liveObjectsPool._onGCInterval = onGCIntervalOriginal; | ||
| } | ||
| }; | ||
| }); | ||
| }; | ||
|
|
||
| await scenario.action({ | ||
| root, | ||
| liveObjectsHelper, | ||
| channelName, | ||
| channel, | ||
| liveObjects, | ||
| helper, | ||
| waitForGCCycles, | ||
| }); | ||
| }, client); | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accessing and modifying private properties and methods in tests
In the test code starting at line 2686, the private properties LiveObjectsPlugin.LiveObjects._DEFAULTS.gcInterval and LiveObjectsPlugin.LiveObjects._DEFAULTS.gcGracePeriod are being modified, and private methods like _onGCInterval are being overridden. Accessing private APIs directly can lead to fragile tests that are tightly coupled to internal implementations. Consider refactoring the code to use public APIs or introduce appropriate hooks or configuration options to facilitate testing without relying on private internals.
| * | ||
| * Applies both for map entries tombstones and object tombstones. | ||
| */ | ||
| gcGracePeriod: 1000 * 60 * 2.5, // 2.5 minutes |
There was a problem hiding this comment.
I think this actually needs to be 24 hours, which is how long realtime will keep tombstones for
| * Even though the `timeserial` from the operation that deleted the object contains the timestamp value, | ||
| * the `timeserial` should be treated as an opaque string on the client, meaning we should not attempt to parse it. | ||
| * | ||
| * Therefore, we need to set our own timestamp when the object is deleted client-side. Strictly speaking, this is |
There was a problem hiding this comment.
This does make an assumption about the client clock not being too heavily skewed behind the server.
I think we have two options for a proper fix:
- Use the
/timeendpoint to get a server timestamp instead ofDate.now() - Include the time component of the timeserial in the
MAP_REMOVEoperation so that the client has access to that value without parsing the timeserial
Given the grace period, the likelihood of encountering a race here is pretty low. So, I think we should create a ticket to do this properly but for now this is fine.
Resolves DTP-1024
Summary by CodeRabbit
New Features
DEFAULTSfor garbage collection settings.LiveCounter,LiveMap,LiveObject, andLiveObjectsPool.Bug Fixes
Tests
Chores