Skip to content
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

Clarify reason for bundling constraint; add contiguousness constraint #521

Merged
merged 2 commits into from
Oct 2, 2018

Conversation

SimonWoolf
Copy link
Member

As suggested in https://ably-real-time.slack.com/archives/C030C5YLY/p1537879666000200 .

Also added RTL6d3 to make the preserve-ordering constraint explicit (I guess we didn't think to include originally because it was too obvious to mention, but better to be explicit)

@paddybyers
Copy link
Member

Is there an argument also for not bundling messages for channel A if there is an intervening message for channel B?

@mattheworiordan
Copy link
Member

Is there an argument also for not bundling messages for channel A if there is an intervening message for channel B?

But a ProtocolMessage applies to only one channel, so how would the library do anything but that?

@paddybyers
Copy link
Member

But a ProtocolMessage applies to only one channel, so how would the library do anything but that?

If the sequence of published messages is 1 (channel A), 2 (channel B), 3 (channel A), the library might bundle 1 and 3 into a single protocol message.

@mattheworiordan
Copy link
Member

If the sequence of published messages is 1 (channel A), 2 (channel B), 3 (channel A), the library might bundle 1 and 3 into a single protocol message.

Well we don't have an API yet to publish messages not on channels (this being discussed in client Transient publishing for 1.2, so should we not add a note in there as that is the only API that would allow this, or am I missing how this would be relevant with the current API?

@SimonWoolf
Copy link
Member Author

SimonWoolf commented Sep 26, 2018

Is there an argument also for not bundling messages for channel A if there is an intervening message for channel B?

FWIW ably-js currently satisfies that anyway, just because its queue function only ever checks the most recent message in the queue. But I also don't follow why it'd be a problem if a client library did do it, given we don't preserve message ordering across channels anyway

should we not add a note in there as that is the only API that would allow this, or am I missing how this would be relevant with the current API?

I don't see how the different publish api is relevant?

@paddybyers
Copy link
Member

But I also don't follow why it'd be a problem if a client library did do it, given we don't preserve message ordering across channels anyway

Just because you will have messages from that connection whose msgSerial is not in the same order as the client requested their publication.

@SimonWoolf
Copy link
Member Author

ok. not sure it matters much, but shrug, happy to add it if you like, no strong feelings

@paddybyers
Copy link
Member

I do think we should impose that constraint

@@ -502,7 +502,8 @@ h3(#realtime-channel). Channel
*** @(RTL6c5)@ A publish should not trigger an implicit attach (in contrast to earlier version of this spec)
** @(RTL6d)@ Messages for a single channel that have been queued may be sent in a single @ProtocolMessage@ by bundling them into the @ProtocolMessage#messages@ array, subject to the following constraints:
*** @(RTL6d1)@ The resulting @ProtocolMesssage@ must not exceed the @maxMessageSize@
*** @(RTL6d2)@ Messages with differing @clientId@ values must not be bundled together
*** @(RTL6d2)@ Messages with differing @clientId@ values must not be bundled together. (Note that this constraint only applies to what the client library can autonomously do as part of queuing messages, not to what the user can do by publishing an array of @Messages@. It exists because if any @Message@ in a @ProtocolMessage@ has an invalid @clientId@, the entire @ProtocolMessage@ is rejected. This is fine if the user has deliberately published the @Messages@ together – they requested atomicity – but not if the client library has bundled them without the user's knowledge)
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be explicit that messages can only be bundled if their clientIds are identical, or identically empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I see the ambiguity (spec says "differing clientId values", "foo" and null are definitely not the same value). But shrug, guess it doesn't do any harm to be overexplicit

@SimonWoolf SimonWoolf force-pushed the bundling-clarification branch from 69bb6f1 to 80fc3dc Compare October 1, 2018 12:07
@SimonWoolf
Copy link
Member Author

Latest: 80fc3dc

Copy link
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

This looks great 👍

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@SimonWoolf SimonWoolf merged commit 3c607f7 into master Oct 2, 2018
@SimonWoolf SimonWoolf deleted the bundling-clarification branch October 2, 2018 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants