[AIT-280] Apply LiveObjects operations on ACK#419
[AIT-280] Apply LiveObjects operations on ACK#419lawrence-forooghian wants to merge 1 commit intomainfrom
Conversation
lawrence-forooghian
changed the base branch from
main
to
clarify-BufferedObjectsOperations-scope
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
f54eee0 to
09a0dad
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
2 times, most recently
from
78c6b72 to
12b5ae7
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
12b5ae7 to
de02ff2
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
d043e79 to
c3a4917
Compare
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
3 times, most recently
from
c3a4917 to
d809334
Compare
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
TODO remove testing plan Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
TODO remove testing plan Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
TODO remove testing plan Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
| ** @(RTO20a)@ Expects the following arguments: | ||
| *** @(RTO20a1)@ @ObjectMessage[]@ - an array of @ObjectMessage@ to be published on a channel | ||
| ** @(RTO20b)@ Calls @RealtimeObjects#publish@ ("RTO15":#RTO15) with the provided @ObjectMessage[]@ and awaits the @PublishResult@. If @publish@ fails, rethrow the error and do not proceed | ||
| ** @(RTO20c)@ If @siteCode@ is not available from "CD2j":../features#CD2j @ConnectionDetails.siteCode@, the client library must throw an @ErrorInfo@ error with @statusCode@ 400 and @code@ 40000 |
There was a problem hiding this comment.
To reviewers: any thoughts on the most appropriate error code for this and RTO20d1?
There was a problem hiding this comment.
I think we can create a new code for this, not sure about status code. I believe this error indicates that we received something wrong from server (so maybe it should be 500)
There was a problem hiding this comment.
Definitely must be in the 500 namespace. Server must provide a siteCode.
Not sure if it justifies a whole separate status code just for a missing field, if were to add one it would be more generic anyway.
I think we can just use 50000 and 500 here.
For RTO20d1 it's mostly the same, however, I wonder if there are legitimate situations where a server might not send PublishResult with a serials field. If it's always expected, but we receive none - then it's a server error, so we can use 50000 and 500. Might be worth checking with a realtime team if serials is always expected. Somewhat related to the comment https://github.com/ably/ably-js/pull/2155/changes#r2798441959
There was a problem hiding this comment.
Remember that if we have a distinct code, we can have a metric for how often that specific error happens. So I would err (no pun) on the side of having new codes where we're unsure.
Might be worth checking with a realtime team if serials is always expected
For v5 and above you should assume you get a serials I think
| **** @(RTO20d2b)@ @ObjectMessage.siteCode@ to the "CD2j":../features#CD2j @ConnectionDetails.siteCode@ | ||
| *** @(RTO20d3)@ Add the synthetic @ObjectMessage@ to the list | ||
| ** @(RTO20e)@ If the "RTO17":#RTO17 sync state is not @SYNCED@: | ||
| *** @(RTO20e1)@ Add an entry to the @bufferedAcks@ list ("RTO7c":#RTO7c) containing the list of synthetic @ObjectMessages@ and a platform-specific signal object |
There was a problem hiding this comment.
To reviewers: After writing this and doing the JS implementation, I wondered whether we really need this bufferedAcks list (and even more so whether we need to specify it). We could instead just say that if if the state isn't SYNCED, to wait until it becomes SYNCED before proceeding to the application of the operation (similarly to waiting to become SYNCED before returning from .getRoot() in RTO1c). Wanted to check people would prefer this before I change it.
There was a problem hiding this comment.
Agree, I am not sure bufferedAcks is needed, without it implementation should be cleaner in my opinion
There was a problem hiding this comment.
Agree, application of buffered acks is always going to happen at the end of sync sequence and there is no special interaction needed when a new sync sequence starts (think regular buffered object operations received during sync - we buffer them in an array, but that is justified as we need to clear that buffered array on the start of new sync sequences).
Written by Claude based on LODR-054 [1]. [1] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
d809334 to
9bd747b
Compare
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Overall looks good! I personally prefer avoiding bufferedAcks and wait for synced instead
| **** @(RTO20d2b)@ @ObjectMessage.siteCode@ to the "CD2j":../features#CD2j @ConnectionDetails.siteCode@ | ||
| *** @(RTO20d3)@ Add the synthetic @ObjectMessage@ to the list | ||
| ** @(RTO20e)@ If the "RTO17":#RTO17 sync state is not @SYNCED@: | ||
| *** @(RTO20e1)@ Add an entry to the @bufferedAcks@ list ("RTO7c":#RTO7c) containing the list of synthetic @ObjectMessages@ and a platform-specific signal object |
There was a problem hiding this comment.
Agree, I am not sure bufferedAcks is needed, without it implementation should be cleaner in my opinion
| ** @(RTO20a)@ Expects the following arguments: | ||
| *** @(RTO20a1)@ @ObjectMessage[]@ - an array of @ObjectMessage@ to be published on a channel | ||
| ** @(RTO20b)@ Calls @RealtimeObjects#publish@ ("RTO15":#RTO15) with the provided @ObjectMessage[]@ and awaits the @PublishResult@. If @publish@ fails, rethrow the error and do not proceed | ||
| ** @(RTO20c)@ If @siteCode@ is not available from "CD2j":../features#CD2j @ConnectionDetails.siteCode@, the client library must throw an @ErrorInfo@ error with @statusCode@ 400 and @code@ 40000 |
There was a problem hiding this comment.
I think we can create a new code for this, not sure about status code. I believe this error indicates that we received something wrong from server (so maybe it should be 500)
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
VeskeR
left a comment
VeskeR
left a comment
There was a problem hiding this comment.
Overall looks good. Couple of comments and suggestions
| **** @(RTO20d2b)@ @ObjectMessage.siteCode@ to the "CD2j":../features#CD2j @ConnectionDetails.siteCode@ | ||
| *** @(RTO20d3)@ Add the synthetic @ObjectMessage@ to the list | ||
| ** @(RTO20e)@ If the "RTO17":#RTO17 sync state is not @SYNCED@: | ||
| *** @(RTO20e1)@ Add an entry to the @bufferedAcks@ list ("RTO7c":#RTO7c) containing the list of synthetic @ObjectMessages@ and a platform-specific signal object |
There was a problem hiding this comment.
Agree, application of buffered acks is always going to happen at the end of sync sequence and there is no special interaction needed when a new sync sequence starts (think regular buffered object operations received during sync - we buffer them in an array, but that is justified as we need to clear that buffered array on the start of new sync sequences).
| * @(RTO19)@ @RealtimeObjects#off@ function - deregisters an event listener previously registered via @RealtimeObjects#on@ ("RTO18":#RTO18) | ||
| * @(RTO22)@ @ObjectsOperationSource@ is an internal enum describing the source of an operation being applied: | ||
| ** @(RTO22a)@ @LOCAL@ - an operation that originated locally, being applied upon receipt of the @ACK@ from Realtime | ||
| ** @(RTO22b)@ @CHANNEL@ - an operation received over a channel attachment |
There was a problem hiding this comment.
| ** @(RTO22b)@ @CHANNEL@ - an operation received over a channel attachment | |
| ** @(RTO22b)@ @CHANNEL@ - an operation received over a channel |
There was a problem hiding this comment.
has nothing to do with an attachment
| ** @(RTO20a)@ Expects the following arguments: | ||
| *** @(RTO20a1)@ @ObjectMessage[]@ - an array of @ObjectMessage@ to be published on a channel | ||
| ** @(RTO20b)@ Calls @RealtimeObjects#publish@ ("RTO15":#RTO15) with the provided @ObjectMessage[]@ and awaits the @PublishResult@. If @publish@ fails, rethrow the error and do not proceed | ||
| ** @(RTO20c)@ If @siteCode@ is not available from "CD2j":../features#CD2j @ConnectionDetails.siteCode@, the client library must throw an @ErrorInfo@ error with @statusCode@ 400 and @code@ 40000 |
There was a problem hiding this comment.
Definitely must be in the 500 namespace. Server must provide a siteCode.
Not sure if it justifies a whole separate status code just for a missing field, if were to add one it would be more generic anyway.
I think we can just use 50000 and 500 here.
For RTO20d1 it's mostly the same, however, I wonder if there are legitimate situations where a server might not send PublishResult with a serials field. If it's always expected, but we receive none - then it's a server error, so we can use 50000 and 500. Might be worth checking with a realtime team if serials is always expected. Somewhat related to the comment https://github.com/ably/ably-js/pull/2155/changes#r2798441959
| *** @(RTO20a1)@ @ObjectMessage[]@ - an array of @ObjectMessage@ to be published on a channel | ||
| ** @(RTO20b)@ Calls @RealtimeObjects#publish@ ("RTO15":#RTO15) with the provided @ObjectMessage[]@ and awaits the @PublishResult@. If @publish@ fails, rethrow the error and do not proceed | ||
| ** @(RTO20c)@ If @siteCode@ is not available from "CD2j":../features#CD2j @ConnectionDetails.siteCode@, the client library must throw an @ErrorInfo@ error with @statusCode@ 400 and @code@ 40000 | ||
| ** @(RTO20d)@ Create a list of synthetic inbound @ObjectMessages@. For each @ObjectMessage@ in the array, paired with the corresponding serial from @PublishResult.serials@ at the same index: |
There was a problem hiding this comment.
I'd prefer this to be clearer to which array we refer to in the sentence "For each @ObjectMessage@ in the array" - it refers to the @ObjectMessage[]@ argument provided to this function, not the list of synthetic inbound @ObjectMessages@ introduced in the previous sentence.
| ** @(RTLC6h)@ Calculate the diff between @previousData@ from "RTLC6g":#RTLC6g and the current @data@ per "RTLC14":#RTLC14, and return the resulting @LiveCounterUpdate@ object | ||
| * @(RTLC7)@ An @ObjectOperation@ from @ObjectMessage.operation@ can be applied to a @LiveCounter@ by performing the following actions in order: | ||
| ** @(RTLC7f)@ Expects the following additional arguments: | ||
| *** @(RTLC7f1)@ @source@ @ObjectsOperationSource@ - the source of the operation (see "RTO22":#RTO22) |
There was a problem hiding this comment.
as we're listing the expected arguments for this operation now, should also mention it expects an ObjectMessage to apply
| ** @(RTLM6h)@ Calculate the diff between @previousData@ from "RTLM6g":#RTLM6g and the current @data@ per "RTLM22":#RTLM22, and return the resulting @LiveMapUpdate@ object | ||
| * @(RTLM15)@ An @ObjectOperation@ from @ObjectMessage.operation@ can be applied to a @LiveMap@ by performing the following actions in order: | ||
| ** @(RTLM15f)@ Expects the following additional arguments: | ||
| *** @(RTLM15f1)@ @source@ @ObjectsOperationSource@ - the source of the operation (see "RTO22":#RTO22) |
There was a problem hiding this comment.
ditto mention expects ObjectMessage
4 participants
Written by Claude based on LODR-054. JS implementation in ably/ably-js#2155.
There are a couple of places here where I'd appreciate input from reviewers; please see the unresolved comments towards the end of the PR.