Batch operations spec points #1421

Peter-Maguire · 2022-05-09T15:04:27Z

NOTE TO REVIEWERS - please do not review PRs in the DRAFT state, as the PR may change substantially before it is ready to review. Thanks.

Description

Adds spec points for the Batch Operations functions.

Review

Page to review

… into feature/sdk-1792-batch-operations # Conflicts: # content/client-lib-development-guide/features.textile

owenpearson

Looks good, just a few comments. Also, I notice that the REST API supports multiple BatchSpec objects in one HTTP request (as in specifying different messages for different channels in the same batch). Did you consider supporting this in SDKs? Even if we don't support it initially it might be good to design this such that we can support it in the future without a breaking change.

content/client-lib-development-guide/features.textile

Co-authored-by: Owen Pearson <48608556+owenpearson@users.noreply.github.com>

Peter-Maguire · 2022-05-11T09:31:14Z

Looks good, just a few comments. Also, I notice that the REST API supports multiple BatchSpec objects in one HTTP request (as in specifying different messages for different channels in the same batch). Did you consider supporting this in SDKs? Even if we don't support it initially it might be good to design this such that we can support it in the future without a breaking change.

@owenpearson this is a good question, I definitely agree that it should be implemented here, although I can't quite figure out how the rest way to do it would be. I've pushed a commit for how I think it should work, could you take a look and see if it's how you were thinking too?

content/client-lib-development-guide/features.textile

owenpearson · 2022-05-11T10:56:51Z

@Peter-Maguire Nice, that is pretty much the API I was imagining, although now that I see it side-by-side with the pre-existing API I almost wonder if it's worth keeping the (string[], Message[]) API, since it would be almost identical to using the (BatchSpec[]) API, for example:

// Using the (string[], Message[]) API
client.batch.publish(['my_cool_channel', 'my_other_cool_channel'], { foo: 'bar' });

// Using the (BatchSpec[]) API
client.batch.publish([{
  channels: ['my_cool_channel', 'my_other_cool_channel'],
  messages: [{ foo: 'bar' }]
}]);

I haven't thought about it too much but my current thinking is maybe we just support two batch.publish signatures: one which takes a single BatchSpec, and another which takes a BatchSpec[]. WDYT?

content/client-lib-development-guide/features.textile

Peter-Maguire · 2022-05-11T11:16:24Z

@owenpearson my initial thinking with including the various inputs for single channels/messages and arrays of them was for convenience. Perhaps having a combination of each plus the BatchSpec methods would be too many different variations, though. Especially for languages that don't support function overloading. I will remove the shorthand functions and replace them with BatchSpec and []BatchSpec, I think that will be easier to implement more consistently.

content/client-lib-development-guide/features.textile

Co-authored-by: Rosie Hamilton <Rosalita@users.noreply.github.com>

content/client-lib-development-guide/features.textile

… into feature/sdk-1792-batch-operations

Rosalita

Thank you for addressing all my comments, I have no more comments to make 👍

owenpearson

LGTM 👍

mattheworiordan · 2022-05-13T15:33:43Z

This PR is great, apologies for feedback after the fact, but I was not aware of this PR until today. It's so great to see the spec evolving 🥳

I do have on question though. Why does the batch method not exist on the Realtime SDK? I believe this creates inconsistency in what we've done historically where all Rest functionality is available in the Realtime SDK by default. As you can see from the screenshot below, by not doing this, we're deviating from what we've done historically. Was this intentional or just an omission?

Peter-Maguire · 2022-05-13T15:37:40Z

@mattheworiordan good point, I assumed that realtime extends rest and that that was defined somewhere in the spec, but I should have checked. As I've already merged this, I can create a new PR to add batch into realtime.

paddybyers · 2022-05-11T07:31:00Z

content/client-lib-development-guide/features.textile

@@ -339,6 +340,14 @@ h3(#rest-encryption). Encryption
 h3(#rest-compatibility). Forwards compatibility
 * @(RSF1)@ The library must apply the "robustness principle":https://en.wikipedia.org/wiki/Robustness_principle in its processing of requests and responses with the Ably system. In particular, deserialization of Messages and related types, and associated enums, must be tolerant to unrecognised attributes or enum values. Such unrecognised values must be ignored.

+h3(#batch-operations). Batch Operations


Would it make more sense for there to be explicit sections for each of the batch operations?

For example an h3 of "Batch publish", and another one for "Batch presence" ?

I'm not sure if I agree with this unless we change the API structure; the h3s in this section of the spec seem to correlate with the API. I've restructured this section in 015aa44 to make it more inline with similar parts of the spec.

paddybyers · 2022-05-11T07:32:36Z

content/client-lib-development-guide/features.textile

@@ -339,6 +340,14 @@ h3(#rest-encryption). Encryption
 h3(#rest-compatibility). Forwards compatibility
 * @(RSF1)@ The library must apply the "robustness principle":https://en.wikipedia.org/wiki/Robustness_principle in its processing of requests and responses with the Ably system. In particular, deserialization of Messages and related types, and associated enums, must be tolerant to unrecognised attributes or enum values. Such unrecognised values must be ignored.

+h3(#batch-operations). Batch Operations
+* @(BO1)@ The batch operations functions must use the REST endpoints in Batch Mode, sending a single request containing all specified data


"must use the REST endpoints in Batch Mode" - what does this mean? I think this spec should be explicit about the client API, and what HTTP request is made as a result (ie specific path, specific params)

Removed this sentence and added some clarity about the request in 015aa44

paddybyers · 2022-05-11T07:35:13Z

content/client-lib-development-guide/features.textile

+** @(BO2a)@ Publishing messages against one or more channels with one or more messages
+*** @(B02a1)@ Functions should be provided to pass either an array or a single object for both Message and Channel
+** @(BO2b)@ Retrieving the presence data for one or more channels
+* @(BO3)@ When all passed arrays contain a single object, the underlying request is functionally identical to it's non-batch equivalent, but the returned result should be a @BatchResponse@ object.


Do you think we should have a generic BatchResponse type, or a BatchPublishResponse ? Or BatchResponse<PublishResult> ?
Edit: I see you have BatchPublishResponse below, but here I'm talking about the outer type representing the top-level response

The outer response is already genericised as BatchResult<BatchPublishResponse>. I'll update this line to reflect that.

paddybyers · 2022-05-11T07:36:21Z

content/client-lib-development-guide/features.textile

+h4. BatchResult
+* @(BPA1)@ Contains the results from the batch operation
+* @(BPA2)@ @BatchResult@ has the following attributes:
+** @(BPA2a)@ @responses@ is an array of batch response objects. undefined if the request failed completely (e.g. for an invalid key)


undefined is a JS concept. I think we need to try to word this in a way that's as language-agnostic as possible

There was a discussion around this here where it was changed to null.

paddybyers · 2022-05-11T07:40:03Z

content/client-lib-development-guide/features.textile

+** @(BPA2a)@ @responses@ is an array of batch response objects. undefined if the request failed completely (e.g. for an invalid key)
+** @(BPA2b)@ @error@ is an @ErrorInfo@ object which is populated if one or more batch publish requests failed
+
+h4. BatchPublishResponse


Note that these individual publish responses are the same thing as you get back from a regular single-channel REST publish request. I know that we hadn't explicitly named that type before, but it might make sense to do that now, and have this as a PublishResponse or PublishResult or something.

This already exists in the java library: https://github.com/ably/ably-java/blob/main/lib/src/main/java/io/ably/lib/types/PublishResponse.java

paddybyers · 2022-05-11T07:41:10Z

content/client-lib-development-guide/features.textile

@@ -1525,6 +1561,7 @@ class Rest:
  constructor(ClientOptions) // RSC1
  auth: Auth // RSC5
  push: Push
+  batch: BatchOperations // BO1


Why is this a client option?
Edit: I misread.
I'm not sure I like the idea that in the API we're grouping together all of the operations that happen to be in batch mode into a top-level entry like this. Publish, presence, and future batch operations (history perhaps) are functionally quite separate - it just happens that they operate in a similar way. This doesn't seem intuitive to me.

I agree, this didn't feel like a 'perfect' solution to me either. I was stuck for a better way to do it though, especially for batch publishing. I could add batch presence to RestPresence easily, but batch publishing to multiple channels didn't feel like it would work in the same way as publishing happens on a Channel object

The only sensible alternative I can think of is to have RestChannels#batchPublish and RestPresence#batchGet, @paddybyers WDYT?

paddybyers · 2022-05-11T07:44:39Z

content/client-lib-development-guide/features.textile

@@ -1697,6 +1735,30 @@ class RealtimeChannel:
  unsubscribe(String, (Message) ->) // RTL8a
  setOptions(options: ChannelOptions) => io // RTL16

+class BatchOperations:


There's an existing Java API for batch publishing: https://github.com/ably/ably-java/blob/main/lib/src/main/java/io/ably/lib/rest/AblyBase.java#L232. Did you look at that?

This existing Java API isn't great, in that you pass in an array of publish specs, and you get back an array of publish responses, and you have to correlate them yourself. (Ie you have to find the publish response that corresponds to the publish spec you passed in). It would be worth thinking about whether or not this API does something to address that problem.

lmars

@Peter-Maguire @owenpearson apologies for being late with my review too! Happy to move the conversation elsewhere, but I thought for now this was the most appropriate place.

lmars · 2022-05-18T21:21:00Z

content/client-lib-development-guide/features.textile

+class BatchPresence:
+  clientId: string
+  action: string?
+  error: ErrorInfo?


I think this error belongs on BatchPresenceResponse

Good spot, I've moved it over on the new PR in a33bfbe and 384b4fe

lmars · 2022-05-18T21:22:56Z

content/client-lib-development-guide/features.textile

 class PushChannel:
  subscribeDevice() => io // RSH7a
  subscribeClient() => io // RSH7b
  unsubscribeDevice() => io // RSH7c
  unsubscribeClient() => io // RSH7d
  listSubscriptions() => io PaginatedResult<PushChannelSubscription> // RSH7e

+class BatchSpec:


I know the documentation calls this BatchSpec too, but it seems specific to publishing a batch of messages, so perhaps it should be called something like BatchPublishRequest?

The batch publish documentation reads as if the BatchSpec object is the one passed into the body of the request either as a single object or within an array as it's being used here. In any case I would argue that the spec should be naming things as consistently as possible with the REST documentation.

In any case I would argue that the spec should be naming things as consistently as possible with the REST documentation.

I agree with that, so I guess what I'm saying is, how about we rename it to BatchPublishRequest in both places? This term isn't something that appears in the API response itself, so we should be free to change it, and now would be the time to change it if we think it could be named better before it starts to appear in SDK code.

I've renamed it to BatchPublishRequest both in the spec and in the docs in a2eee7d

lmars · 2022-05-18T21:25:13Z

content/client-lib-development-guide/features.textile

+
+class BatchPresenceResponse:
+  channel: String // BPD2a
+  presence: []BatchPresence // PBD2b


I think this should be []PresenceMessage, not sure an extra type is needed.

PresenceMessage has far more fields on it than what appears in the response for a batch presence request. If those fields are all optional or can be retrieved from context then I would be happy to use that type instead.

PresenceMessage has far more fields on it than what appears in the response for a batch presence request

Can you give an example? I'm not sure why retrieving presence for multiple channels in one request should return less fields than retrieving for a single channel, if that is the case it sounds more like a problem with the API that we should fix rather than something we should be encoding into the type system.

According to the spec, PresenceMessage has connectionId, id and timestamp along with action and clientId which are the only two fields that the REST API returns for batch presence.
Docs link for PresenceMessage | Docs link for BatchPresence

PresenceMessage has connectionId, id and timestamp along with action and clientId which are the only two fields that the REST API returns for batch presence.

The docs may seem to indicate that, but it's not accurate, you get the same fields from both endpoints:

$ curl -u "${ABLY_API_KEY}" "https://rest.ably.io/channels/test1/presence" [ { "id": "LnUWZJf1G-:0:0", "clientId": "lmars", "connectionId": "LnUWZJf1G-", "timestamp": 1653686295156, "data": "", "action": 1 } ] $ curl -u "${ABLY_API_KEY}" "https://rest.ably.io/presence?channel=test1" [ { "channel": "test1", "presence": [ { "id": "LnUWZJf1G-:0:0", "clientId": "lmars", "connectionId": "LnUWZJf1G-", "timestamp": 1653686295156, "data": "", "action": 1 } ] } ]

I've updated to use PresenceMessage in 384b4fe. I've also created #1451 to address this on the docs side.

lmars · 2022-05-18T21:43:56Z

content/client-lib-development-guide/features.textile

@@ -1697,13 +1735,40 @@ class RealtimeChannel:
  unsubscribe(String, (Message) ->) // RTL8a
  setOptions(options: ChannelOptions) => io // RTL16

+class BatchOperations:
+  publish([BatchSpec]) => BatchResult<BatchPublishResponse> // BO2a


Thinking about partial success, I think I'd like to get back something which helps me understand which of the things I tried to publish failed.

Whilst I understand why the REST API is designed as it is, I think there's an opportunity to wrap it in a client-side abstraction which might be more useful.

For example, what if it looked like this:

batch := client.newBatchPublish() req1 := batch.publish("channel1", "msg1") req2 := batch.publish("channel1", "msg2") req3 := batch.publish("channel2", "msg3") results, err := batch.send()

and results somehow helps me correlate any errors with one or more of req1, req2, or req3.

Batch operations spec points

5ec6d58

Peter-Maguire self-assigned this May 9, 2022

Peter Maguire added 5 commits May 9, 2022 16:43

some headings

57aed76

Batch operations spec points

f4ba9fd

Merge remote-tracking branch 'origin/feature/sdk-1792-batch-operations'…

b407a0d

… into feature/sdk-1792-batch-operations # Conflicts: # content/client-lib-development-guide/features.textile

add batch operations heading for the rest client

5708b1d

Merge branch 'main' into feature/sdk-1792-batch-operations

e5d7e7f

Peter-Maguire marked this pull request as ready for review May 10, 2022 15:00

Peter-Maguire added the client-lib-spec label May 10, 2022

Peter-Maguire requested review from QuintinWillison, Rosalita, owenpearson and lmars May 10, 2022 15:02

fix duplicates

9943b89

owenpearson requested changes May 10, 2022

View reviewed changes

Peter Maguire and others added 3 commits May 11, 2022 10:07

Update content/client-lib-development-guide/features.textile

e1d3090

Co-authored-by: Owen Pearson <48608556+owenpearson@users.noreply.github.com>

suggested changes

2cdba4a

BatchSpec publishing

8599712

Peter-Maguire requested a review from owenpearson May 11, 2022 09:31

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Outdated Show resolved Hide resolved

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Show resolved Hide resolved

Peter Maguire added 2 commits May 11, 2022 12:19

replace references to 'undefined' with 'null'

b0ec997

remove shorthand publish functions

1e1ded8

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Outdated Show resolved Hide resolved

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Outdated Show resolved Hide resolved

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Show resolved Hide resolved

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Outdated Show resolved Hide resolved

Rosalita reviewed May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Show resolved Hide resolved

Update content/client-lib-development-guide/features.textile

16917a8

Co-authored-by: Rosie Hamilton <Rosalita@users.noreply.github.com>

owenpearson requested changes May 11, 2022

View reviewed changes

content/client-lib-development-guide/features.textile Show resolved Hide resolved

Peter Maguire added 2 commits May 12, 2022 16:28

clarify the purpose of BatchResult.error

32d50b8

Merge remote-tracking branch 'origin/feature/sdk-1792-batch-operations'…

076f778

… into feature/sdk-1792-batch-operations

Peter-Maguire requested review from owenpearson and Rosalita May 13, 2022 13:13

Rosalita approved these changes May 13, 2022

View reviewed changes

owenpearson approved these changes May 13, 2022

View reviewed changes

Peter-Maguire merged commit 44d9e76 into main May 13, 2022

Peter-Maguire deleted the feature/sdk-1792-batch-operations branch May 13, 2022 15:00

Peter-Maguire mentioned this pull request May 13, 2022

Add batch operations to realtime #1424

Merged

paddybyers reviewed May 18, 2022

View reviewed changes

lmars reviewed May 18, 2022

View reviewed changes

QuintinWillison mentioned this pull request Oct 17, 2022

Add REST 'Batch Operations' features, including for publish (BO2a) and getPresence (BO2b) ably/features#81

Open

Batch operations spec points #1421

Batch operations spec points #1421

Conversation

Peter-Maguire commented May 9, 2022 • edited

Description

Review

owenpearson left a comment

Choose a reason for hiding this comment

Peter-Maguire commented May 11, 2022

owenpearson commented May 11, 2022

Peter-Maguire commented May 11, 2022

Rosalita left a comment

Choose a reason for hiding this comment

owenpearson left a comment

Choose a reason for hiding this comment

mattheworiordan commented May 13, 2022

Peter-Maguire commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter-Maguire May 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddybyers May 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenpearson Jun 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter-Maguire commented May 9, 2022 •

edited

Peter-Maguire May 26, 2022 •

edited

paddybyers May 11, 2022 •

edited

owenpearson Jun 8, 2022 •

edited