-
Notifications
You must be signed in to change notification settings - Fork 4
[PUB-1828] Add Objects spec for object and map entry tombstones, and OBJECT_DELETE message handling #350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Spec IDs from [1]. [1] ably/specification#350
Spec IDs from [1]. [1] ably/specification#350
This is preparation for adding additional fields (e.g. tombstonedAt) per RTLM3a in [1]. [1] ably/specification#350
Per [1] at 488e932. Preparation for implementing this tombstoning spec. [1] ably/specification#350
Preparation for adding the `tombstone` method from [1]. (This approach is a _bit_ weird but it's what I could think of that's compatible with the existing LiveObjectMutableState approach.) [1] ably/specification#350
This is preparation for tombstoning an object per [1]; we'll need the ObjectMessage's serialTimestamp. [1] ably/specification#350
Preparation for making these methods perform tombstoning per [1] (not implemented yet). [1] ably/specification#350
The logic for this check is going to expand when we implement the tombstoning spec [1], so let's DRY it up. [1] ably/specification#350
The logic for this check is going to expand when we implement the tombstoning spec [1], so let's DRY it up. [1] ably/specification#350
This is preparation for tombstoning an object per [1]; we'll need the ObjectMessage's serialTimestamp. [1] ably/specification#350
Preparation for making these methods perform tombstoning per [1] (not implemented yet). [1] ably/specification#350
The logic for this check is going to expand when we implement the tombstoning spec [1], so let's DRY it up. [1] ably/specification#350
Based on [1] at 488e932. Haven't updated tests due to being in a bit of a rush; deferred to #52. [1] ably/specification#350
Based on [1] at 488e932. Haven't updated tests due to being in a bit of a rush; deferred to #52. [1] ably/specification#350
textile/objects-features.textile
Outdated
| ** @(RTO10a)@ The check should occur at regular intervals, for example, every 5 minutes | ||
| ** @(RTO10b)@ The grace period for releasing resources for tombstoned objects and map entries is determined as follows: | ||
| *** @(RTO10b1)@ It is equal to "@ConnectionDetails.gcGracePeriod@":../features#CD2i received in the @CONNECTED@ @ProtocolMessage@ | ||
| *** @(RTO10b2)@ The grace period value is updated to match the new @ConnectionDetails.gcGracePeriod@ value whenever a new @CONNECTED@ @ProtocolMessage@ is received per "RTN24":../features#RTN24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| *** @(RTO10b2)@ The grace period value is updated to match the new @ConnectionDetails.gcGracePeriod@ value whenever a new @CONNECTED@ @ProtocolMessage@ is received per "RTN24":../features#RTN24 | |
| *** @(RTO10b2)@ The grace period value is updated to match the new @ConnectionDetails. objectsGCGracePeriod@ value whenever a new @CONNECTED@ @ProtocolMessage@ is received per "RTN24":../features#RTN24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, do we really need to update grace period every time connection becomes connected?
Instead can we select value from the first CONNECTED message, because value will remain same for other re-connections right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if connection already in CONNECTED state, then use the stored grace period value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in b66c60a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we always wait for the next CONNECTED event, even if the state is already CONNECTED.
If this is actual implementation in kotlin, then this is incorrect. SDK must store the objectsGCGracePeriod from latest received CONNECTED event and use that, and if it was omitted, use a default value from as specified in RTO10b3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to ably-js implementation. Currently, it waits for next CONNECTED/ connectiondetails event, https://github.com/ably/ably-js/blob/af2ce0eef23068b19cccd2677453386cc470372d/src/plugins/objects/objects.ts#L71. Doesn't check for a case if it's already connected.
am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not wait for anything, that line implements RTO10b2 by subscribing for updates to the connection details (which happen on CONNECTED events) and updates locally stored objectsGCGracePeriod as expected by the spec.
There is no explicit need to wait for the first CONNECTED event, as RTO10b3 ensures we have a default value.
You can see it being used at https://github.com/ably/ably-js/blob/af2ce0eef23068b19cccd2677453386cc470372d/src/plugins/objects/objects.ts#L70, where we use objectsGCGracePeriod if we have it already, otherwise use a default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, guess I missed out on that one !
Thanks, this resolves my issue 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
Spec probably can be slightly improved to make it more clear about its intention, created #376 for this as I'd like to merge this PR and not modify it further
58bd559 to
b66c60a
Compare
2963300 to
c63992b
Compare
a1c9a6a to
7149569
Compare
c63992b to
46888e4
Compare
…TE message Resolves PUB-1828
…nd `tombstonedAt` properties
…sGCGracePeriod` The previous `gcGracePeriod` name was never used in production so we can update the spec without deleting the old spec points
…es in LiveObjects spec
7149569 to
b696a9b
Compare
Related DRs: LODR-014 Garbage Collection, LODR-026 Correctness of OBJECT_DELETE
Resolves PUB-1828