Apply timeserial dependant operations only if op timeserial is > than object/entry timeserial #1912
Conversation
WalkthroughThe changes in this pull request involve modifications to the 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/1912/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1912/features
github-actions
bot
temporarily deployed
to
staging/pull/1912/typedoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/plugins/liveobjects/liveobjectspool.ts (1)
163-163: Consider improving log message readability.The log message could be more readable by restructuring the comparison information.
Consider this format:
- `skipping applying buffered state operation message as existing object has greater or equal regional timeserial: ${existingObject.getRegionalTimeserial().toString()}, than the op: ${regionalTimeserial.toString()}; objectId=${stateMessage.operation.objectId}, message id: ${stateMessage.id}, channel: ${this._channel.name}`, + `skipping buffered state operation (objectId=${stateMessage.operation.objectId}, msgId=${stateMessage.id}): existing timeserial (${existingObject.getRegionalTimeserial().toString()}) >= op timeserial (${regionalTimeserial.toString()}); channel=${this._channel.name}`,src/plugins/liveobjects/livemap.ts (2)
227-236: LGTM! Consider minor log message improvement.The updated condition correctly implements the requirement to ignore operations when their timeserial is less than or equal to the existing entry's timeserial. This helps prevent duplicate operations and maintains consistency.
Consider making the log message slightly more concise:
- `skipping updating key="${op.key}" as existing key entry has greater or equal timeserial: ${existingEntry.timeserial.toString()}, than the op: ${opOriginTimeserial.toString()}; objectId=${this._objectId}`, + `skipping update for key="${op.key}": existing timeserial (${existingEntry.timeserial.toString()}) >= op timeserial (${opOriginTimeserial.toString()}); objectId=${this._objectId}`,
277-286: LGTM! Consider consistent log message improvement.The changes correctly mirror the timeserial handling in _applyMapSet, maintaining consistency in how both set and remove operations are handled.
For consistency with the previous suggestion, consider updating the log message:
- `skipping removing key="${op.key}" as existing key entry has greater or equal timeserial: ${existingEntry.timeserial.toString()}, than the op: ${opOriginTimeserial.toString()}; objectId=${this._objectId}`, + `skipping remove for key="${op.key}": existing timeserial (${existingEntry.timeserial.toString()}) >= op timeserial (${opOriginTimeserial.toString()}); objectId=${this._objectId}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/plugins/liveobjects/livemap.ts (2 hunks)
- src/plugins/liveobjects/liveobjectspool.ts (1 hunks)
- test/realtime/live_objects.test.js (2 hunks)
🔇 Additional comments (4)
src/plugins/liveobjects/liveobjectspool.ts (1)
154-157: LGTM! Verify consistency across components.The addition of the equality check in the timeserial comparison aligns with the PR objective and prevents duplicate operations when timeserials are equal. This is a logical improvement to ensure operations are only applied when they have strictly newer timeserials.
Let's verify that this behavior is consistently implemented across related components:
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response:
The timeserial comparison behavior is consistently implemented across the codebase
The equality check addition in
liveobjectspool.tsaligns with existing implementations across the codebase:
LiveMap._applyMapSet()andLiveMap._applyMapRemove()already use the same pattern:(opOriginTimeserial.before(existingEntry.timeserial) || opOriginTimeserial.equal(existingEntry.timeserial))
The
Timeserialinterface and its implementation inDefaultTimeserialclass properly support all comparison operations (before,equal,after) with consistent behavior.All live objects (LiveMap, LiveCounter) use the same timeserial comparison logic for their operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar timeserial comparison patterns in related files rg -A 2 "timeserial\.(before|equal)" src/plugins/liveobjects/Length of output: 63
Script:
#!/bin/bash # Let's try a broader search for timeserial usage patterns rg -i "timeserial" src/plugins/liveobjects/ -A 2 # Also search for the Timeserial class/type definition ast-grep --pattern 'class $_ { $$$ before($_) { $$$ } $$$ }'Length of output: 30004
src/plugins/liveobjects/livemap.ts (1)
227-230: Verify consistent timeserial handling across codebase.The changes implement timeserial equality checks consistently in both methods. Let's verify this pattern is followed in related classes.
Also applies to: 277-280
✅ Verification successful
Timeserial comparison implementation is consistent across codebase
The codebase search reveals that timeserial handling is implemented consistently:
- The
Timeserialinterface and its implementation inDefaultTimeserialclass provides the core comparison methods (before,equal,after) used throughout the codebase- The comparison logic in
livemap.tscorrectly uses these methods to handle operation ordering- The pattern
timeserial.before() || timeserial.equal()is used consistently for checking if an operation should be skipped- Zero-value timeserial is properly used as the default when no timeserial is provided
The implementation in the reviewed code follows the established patterns and is consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar timeserial comparison patterns in related files echo "Checking for timeserial comparison patterns:" rg -A 2 "timeserial\.(before|equal)" src/plugins/liveobjects/ echo -e "\nChecking for test coverage of timeserial equality cases:" rg -A 5 "test.*timeserial.*equal" src/plugins/liveobjects/Length of output: 352
Script:
#!/bin/bash # Let's try a more comprehensive search for timeserial-related code echo "Searching for timeserial usage patterns:" rg -A 3 "timeserial" src/plugins/liveobjects/ echo -e "\nSearching for test files:" fd "test" src/plugins/liveobjects/ echo -e "\nLooking for Timeserial class definition:" ast-grep --pattern 'class Timeserial { $$$ }'Length of output: 19884
test/realtime/live_objects.test.js (2)
Line range hint
1118-1126: Assertions correctly verify that outdated operations are not appliedThe test accurately asserts that operations with older or equal regional timeserials are not applied after the
STATE_SYNCsequence ends, ensuring the state remains consistent as expected.
1076-1093: 🛠️ Refactor suggestionReplace
flatMap()to ensure broader compatibilityThe use of
Array.prototype.flatMap()may not be supported in environments that do not implement ES2019. To maintain compatibility with older environments, consider replacingflatMap()with nestedmap()functions combined withconcat()or usingreduce().You can refactor the code as follows:
-await Promise.all( - ['root', mapId].flatMap((objectId) => - primitiveKeyData.map((keyData) => - liveObjectsHelper.processStateOperationMessageOnChannel({ - channel, - serial, - state: [liveObjectsHelper.mapSetOp({ objectId, key: keyData.key, data: keyData.data })], - }), - ), - ), -); +const operations = []; +['root', mapId].forEach((objectId) => { + primitiveKeyData.forEach((keyData) => { + operations.push( + liveObjectsHelper.processStateOperationMessageOnChannel({ + channel, + serial, + state: [liveObjectsHelper.mapSetOp({ objectId, key: keyData.key, data: keyData.data })], + }), + ); + }); +}); +await Promise.all(operations);Likely invalid or redundant comment.
VeskeR
force-pushed
the
liveobjects/timeserials-comparison-change
branch
from
911b9e3 to
73d2ad9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1912/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1912/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1912/features
VeskeR
force-pushed
the
DTP-955/buffer-ops-during-sync
branch
from
0a3ff83 to
ef7b975
Compare
VeskeR
force-pushed
the
liveobjects/timeserials-comparison-change
branch
from
73d2ad9 to
e763a30
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1912/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1912/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1912/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/realtime/live_objects.test.js (2)
1073-1092: Consider refactoring the nested Promise.all structure for better readability and error handling.The nested Promise.all structure makes it harder to track failures and debug issues. Consider restructuring this to be more linear and explicit.
- await Promise.all( - ['@0-0', '@1-0'].map(async (serial) => { - await Promise.all( - ['root', mapId].flatMap((objectId) => - primitiveKeyData.map((keyData) => - liveObjectsHelper.processStateOperationMessageOnChannel({ - channel, - serial, - state: [liveObjectsHelper.mapSetOp({ objectId, key: keyData.key, data: keyData.data })], - }), - ), - ), - ); - await liveObjectsHelper.processStateOperationMessageOnChannel({ - channel, - serial, - state: [liveObjectsHelper.counterIncOp({ objectId: counterId, amount: 1 })], - }); - }), - ); + // Process operations for each timeserial sequentially + for (const serial of ['@0-0', '@1-0']) { + // Process map operations + const mapOperations = ['root', mapId].flatMap((objectId) => + primitiveKeyData.map((keyData) => ({ + channel, + serial, + state: [liveObjectsHelper.mapSetOp({ objectId, key: keyData.key, data: keyData.data })] + })) + ); + await Promise.all( + mapOperations.map(op => + liveObjectsHelper.processStateOperationMessageOnChannel(op) + ) + ); + + // Process counter operation + await liveObjectsHelper.processStateOperationMessageOnChannel({ + channel, + serial, + state: [liveObjectsHelper.counterIncOp({ objectId: counterId, amount: 1 })] + }); + }
Line range hint
1117-1126: Enhance test assertions with more explicit timeserial comparison context.The test assertions could be more explicit about the timeserial comparison being the reason for operations not being applied.
- // check operations with older or equal regional timeserial are not applied - // counter will be checked to match an expected value explicitly, so no need to check that it doesn't equal a sum of operations + // Verify that operations with timeserials '@0-0' and '@1-0' (older or equal to '@1-0') are not applied + // This validates the timeserial comparison logic where operations are only applied if their timeserial is greater primitiveKeyData.forEach((keyData) => { expect( root.get(keyData.key), - `Check "${keyData.key}" key doesn't exist on root when STATE_SYNC has ended`, + `Check "${keyData.key}" key doesn't exist on root as its operation timeserial (@0-0, @1-0) was not greater than object timeserial (@1-0)`, ).to.not.exist; }); primitiveKeyData.forEach((keyData) => { expect( root.get('map').get(keyData.key), - `Check "${keyData.key}" key doesn't exist on inner map when STATE_SYNC has ended`, + `Check "${keyData.key}" key doesn't exist on inner map as its operation timeserial (@0-0, @1-0) was not greater than object timeserial (@1-0)`, ).to.not.exist; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/plugins/liveobjects/livemap.ts(2 hunks)src/plugins/liveobjects/liveobjectspool.ts(1 hunks)test/realtime/live_objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/livemap.ts
- src/plugins/liveobjects/liveobjectspool.ts
VeskeR
force-pushed
the
DTP-955/buffer-ops-during-sync
branch
from
f44b2d2 to
d919f8b
Compare
… object/entry timeserial
VeskeR
force-pushed
the
liveobjects/timeserials-comparison-change
branch
from
e763a30 to
868560c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/realtime/live_objects.test.js (2)
1073-1092: LGTM! Comprehensive test coverage for timeserial comparison.The test thoroughly validates that operations with older (@0-0) or equal (@1-0) regional timeserials are not applied during sync. The parallel execution using Promise.all is efficient.
Consider extracting the timeserial values into named constants for better readability:
- ['@0-0', '@1-0'].map(async (serial) => { + const OLDER_TIMESERIAL = '@0-0'; + const EQUAL_TIMESERIAL = '@1-0'; + [OLDER_TIMESERIAL, EQUAL_TIMESERIAL].map(async (serial) => {
Line range hint
1117-1134: LGTM! Thorough verification of timeserial-based operation filtering.The verification logic is comprehensive, checking both the root and inner map to ensure operations with older/equal timeserials were not applied. The explicit error messages make test failures easy to debug.
Consider extracting the verification logic into a helper function to reduce code duplication:
+ function verifyKeyNotExists(obj, keyData, context) { + expect( + obj.get(keyData.key), + `Check "${keyData.key}" key doesn't exist on ${context} when STATE_SYNC has ended`, + ).to.not.exist; + } + primitiveKeyData.forEach((keyData) => { - expect( - root.get(keyData.key), - `Check "${keyData.key}" key doesn't exist on root when STATE_SYNC has ended`, - ).to.not.exist; + verifyKeyNotExists(root, keyData, 'root'); }); primitiveKeyData.forEach((keyData) => { - expect( - root.get('map').get(keyData.key), - `Check "${keyData.key}" key doesn't exist on inner map when STATE_SYNC has ended`, - ).to.not.exist; + verifyKeyNotExists(root.get('map'), keyData, 'inner map'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/plugins/liveobjects/livemap.ts(2 hunks)src/plugins/liveobjects/liveobjectspool.ts(1 hunks)test/realtime/live_objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/livemap.ts
- src/plugins/liveobjects/liveobjectspool.ts
This reverts the change for timeserial comparison suggested in comment.
The intended way is to expect an operation to have > timeserial than the object/entry. See internal slack conversation.
Also applies the same change for regional timeserial comparison when applying buffered operation messages
Summary by CodeRabbit
New Features
Bug Fixes
Tests