Skip to content
This repository was archived by the owner on Aug 10, 2026. It is now read-only.

Initial attempt on delta related changes specification - #779

Merged
codemerx merged 5 commits into
master-1-2from
feature-deltas
Dec 19, 2019
Merged

Initial attempt on delta related changes specification#779
codemerx merged 5 commits into
master-1-2from
feature-deltas

Conversation

@codemerx

Copy link
Copy Markdown
Contributor

Attempts to specify the changes to the client library introduced by the deltas feature. There would include deltas, channel params, bits of rewind, etc.

This PR is based on the discussions and materials at https://github.com/ably/wiki/pull/391 and https://github.com/ably/wiki/issues/380.

@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 November 27, 2019 17:55 Inactive

@SimonWoolfSimonWoolf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think perhaps we haven't been clear enough about the purpose of the features spec -- our fault, apologies. This PR seems to be treating it as something like a raw form of our user documentation, but it isn't that. Rather it's a list of client library features with specifications for each one, that a client library developer can go through and implement. (We have a spreadsheet where we track which items are implemented by which client library). Conformance to a version of the spec item is how we determine if a client library is ready to be released with a particular version number.

Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 November 28, 2019 15:28 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 November 28, 2019 16:07 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 November 28, 2019 18:27 Inactive

@SimonWoolfSimonWoolf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking much better 🙂

Still to do: specify the messageId bookkeeping (extras.delta.from !== this._lastPayload.messageId etc)

Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile
Comment threadcontent/client-lib-development-guide/features.textile
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 November 29, 2019 17:00 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 2, 2019 13:42 Inactive
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated

@mattheworiordanmattheworiordan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various comments. One issue I have is that this PR is not based against the 1.2 WIP spec, so when I try and reference spec items such as RTL4c, which I would assume cover off how ChannelOptions are sent to Ably (it does exist, but covered un RTL4j now in #766), it is out of date, and such this PR cannot really be reviewed in full. Why was it not based against #766 (branch channel-params).

Also, as a general note:

  • Please remember to use @ symbols to surround code symbols such as ATTACH or ProtocolMessage
  • I think we can do better at helping developers who read this spec to understand not just what is required, but also to help test it. We are looking for consistency across our client libraries not just in the assumed code and behaviours, but also in out tests.

Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 3, 2019 15:08 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 3, 2019 15:17 Inactive
@codemerx
codemerx changed the base branch from master to master-1-2December 3, 2019 15:18
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 4, 2019 16:15 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 4, 2019 16:23 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 4, 2019 20:03 Inactive
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile
codemerx pushed a commit to ably/ably-java that referenced this pull request Dec 5, 2019
@marto83

Copy link
Copy Markdown
Contributor

I’m looking at the spec items about delta recovery. The spec makes it clear about what I need to do in the happy path where the recovery succeeds. However it doesn't cover what happens if the recovery fails.
Let’s stay we start a recovery and send a new ATTACH message but that times out - the default behaviour will kick in and the channel will become SUSPENDED. What happens when we re-attach it (no matter who triggered that). Are we still in recovery mode and send the serial of the last message? Or do we forget about it and you just get attach from that moment on.

@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 6, 2019 19:30 Inactive
@codemerx

Copy link
Copy Markdown
ContributorAuthor

I’m looking at the spec items about delta recovery. The spec makes it clear about what I need to do in the happy path where the recovery succeeds. However it doesn't cover what happens if the recovery fails.
Let’s stay we start a recovery and send a new ATTACH message but that times out - the default behaviour will kick in and the channel will become SUSPENDED. What happens when we re-attach it (no matter who triggered that). Are we still in recovery mode and send the serial of the last message? Or do we forget about it and you just get attach from that moment on.

We do not specify recovery mode for the channel. We specify recovery actions that should be taken. If the ATTACH never results in ATTACHED reply then one just follows the spec, i.e. the channel becomes SUSPENDED and so on. Whatever happens to it later on should not depend on the failed delta decoding recovery attempt.

@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 6, 2019 19:46 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 13, 2019 14:59 Inactive
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 13, 2019 15:10 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 16, 2019 11:13 Inactive
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile
Comment threadcontent/client-lib-development-guide/features.textile Outdated
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 16, 2019 13:18 Inactive
@mattheworiordan
mattheworiordan temporarily deployed to ably-docs-staging-pr-779 December 17, 2019 18:57 Inactive

@paddybyerspaddybyers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments but otherwise I think this is ready to merge.

Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
Comment threadcontent/client-lib-development-guide/features.textile Outdated
@codemerx
codemerx marked this pull request as ready for review December 19, 2019 14:59
@codemerx
codemerx merged commit a50a744 into master-1-2Dec 19, 2019
codemerx pushed a commit that referenced this pull request Dec 19, 2019
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit that referenced this pull request Mar 9, 2020
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit that referenced this pull request Mar 23, 2020
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit that referenced this pull request Apr 3, 2020
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit that referenced this pull request Apr 7, 2020
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit that referenced this pull request Apr 15, 2020
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per #779 (comment)
QuintinWillison pushed a commit to ably/specification that referenced this pull request Sep 20, 2022
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per ably/docs#779 (comment)
ttypic pushed a commit to ably/specification that referenced this pull request Mar 6, 2026
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per ably/docs#779 (comment)
ttypic pushed a commit to ably/specification that referenced this pull request Mar 6, 2026
Initial attempt on deltas specification
Removing duplicate spec
Enhancing delta decode recovery spec
Added delta bookkeping and pluggable codecs spec
Fixes based on PR review.
PR comment based fixes
Replacing pluggable decoders by plugins
Minor fixes based on PR comments
Fixes based on PR comments
Making the spec for delta failure recovery more precise.
Minor fixes based on PR review
Removing a rule that should never kick in if messages ignored when ATTACHING
Vcdiff deltas: various clarifications
Change as per ably/docs#779 (comment)
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@codemerx@marto83@mattheworiordan@paddybyers@SimonWoolf@QuintinWillison@tsviatko