Skip to content

Batch operations spec points #1421

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.

Sign up for GitHub

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

Merged
Peter-Maguire merged 15 commits into from
May 13, 2022
Merged

Batch operations spec points #1421

Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5ec6d58
Batch operations spec points
May 9, 2022
57aed76
some headings
May 9, 2022
f4ba9fd
Batch operations spec points
May 9, 2022
b407a0d
Merge remote-tracking branch 'origin/feature/sdk-1792-batch-operation…
May 10, 2022
5708b1d
add batch operations heading for the rest client
May 10, 2022
e5d7e7f
Merge branch 'main' into feature/sdk-1792-batch-operations
May 10, 2022
9943b89
fix duplicates
May 10, 2022
e1d3090
Update content/client-lib-development-guide/features.textile
May 11, 2022
2cdba4a
suggested changes
May 11, 2022
8599712
BatchSpec publishing
May 11, 2022
b0ec997
replace references to 'undefined' with 'null'
May 11, 2022
1e1ded8
remove shorthand publish functions
May 11, 2022
16917a8
Update content/client-lib-development-guide/features.textile
May 11, 2022
32d50b8
clarify the purpose of BatchResult.error
May 12, 2022
076f778
Merge remote-tracking branch 'origin/feature/sdk-1792-batch-operation…
May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 68 additions & 3 deletions content/client-lib-development-guide/features.textile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ jump_to:
- Presence#rest-presence
- Encryption#rest-encryption
- Forwards compatibility#rest-compatibility
- Batch Operations#batch-operations
Realtime client library:
- RealtimeClient
- Connection#realtime-connection
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Member

@paddybyers paddybyers May 11, 2022

Choose a reason for hiding this comment

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

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" ?

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022

Choose a reason for hiding this comment

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

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.

* @(BO1)@ The batch operations functions must use the REST endpoints in Batch Mode, sending a single request containing all specified data
Copy link
Copy Markdown
Member

@paddybyers paddybyers May 11, 2022

Choose a reason for hiding this comment

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

"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)

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022

Choose a reason for hiding this comment

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

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

* @(BO2)@ Batch operations must be able to be performed for the following:
** @(BO2a)@ @BatchOperations::publish@ publishes messages against one or more channels with one or more messages
*** @(B02a1)@ Functions should be provided to pass either an array or a single @BatchSpec@ object. In languages where function overloading is not possible, an array is preferred.
** @(BO2b)@ @BatchOperations::getPresence@ retrieves the presence data for one or more channels
* @(BO3)@ When a batch operation only contains one batch, the underlying request is functionally identical to its non-batch equivalent, but the returned result should be a @BatchResponse@ object.

h2(#realtime). Realtime client library features

The Ably Realtime client libraries establish and maintain a persistent connection to Ably and provide methods to publish and subscribe to messages over a low latency realtime connection.
Expand Down Expand Up @@ -1348,7 +1357,7 @@ h4. ChannelDetails

h4. ChannelStatus

* @(CHS1)@ @ChannelStatus@ is a type that contains status and occupancy for a channel
* @(CHS1)@ @ChannelStatus@ is a type that contains status and occupancy for a channel
* @(CHS2)@ The attributes of @ChannelStatus@ consist of:
** @(CHS2a)@ @isActive@ boolean - represents if the channel is active
** @(CHS2b)@ @occupancy@ @ChannelOccupancy@ - occupancy is an object containing the metrics for the channel
Expand All @@ -1361,15 +1370,43 @@ h4. ChannelOccupancy

h4. ChannelMetrics

* @(CHM1)@ @ChannelMetrics@ is a type that contains the count of publishers and subscribers, connections and presenceConnections, presenceMembers and presenceSubscribers
* @(CHM1)@ @ChannelMetrics@ is a type that contains the count of publishers and subscribers, connections and presenceConnections, presenceMembers and presenceSubscribers
* @(CHM2)@ The attributes of @ChannelMetrics@ consist of:
** @(CHM2a)@ @connections@ integer - the total number of connections to the channel
** @(CHM2b)@ @presenceConnections@ integer - the total number of presence connections to the channel
** @(CHM2c)@ @presenceMembers@ integer - the total number of presence members for the channel
** @(CHM2d)@ @presenceSubscribers@ integer - the total number of presence subscribers for the channel
** @(CHM2e)@ @publishers@ integer - the total number of publishers to the channel
** @(CHM2e)@ @publishers@ integer - the total number of publishers to the channel
** @(CHM2f)@ @subscribers@ integer - the total number of subscribers to the channel

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.
** @(BPA2b)@ @error@ is an @ErrorInfo@ object which is populated if one or more batch publish requests failed.
*** @(BPA2b1)@ This error should only be set if it relates to a partial success. All fatal errors should be handled via language appropriate error handling.

h4. BatchPublishResponse
Copy link
Copy Markdown
Member

@paddybyers paddybyers May 11, 2022

Choose a reason for hiding this comment

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

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

* @(BPB1)@ Contains information for each batch publish request within a @BatchResult@
* @(BPB2)@ @BatchPublishResponse@ has the following attributes:
** (@BPB2a)@ @channel@ is the channel name which this publish request was directed to
** (@BPB2b)@ @messageId@ contains the resultant message ID, if the request succeeds and is null if @error@ is present
** (@BPB2c)@ @error@ contains an @ErrorInfo@ object if this publish request failed, and is null if it succeeded

h4. BatchPresenceResponse
* @(BPD1)@ Contains information for each batch presence request within a @BatchResult@
* @(BPD2)@ @BatchPresenceResponse@ contains the following attributes:
** @(BPD2a)@ @channel@ is the channel name which this presence request
** @(BPD2b)@ @presence@ is an array of presence data for the @channel@

h4. BatchPresence
* @(BPE1)@ Is a partial @PresenceMessage@ object containing @clientId@ and @action@, or @error@ if the presence failed
* @(PBE2)@ @BatchPresence@ contains the following attributes:
** @(PBE2a)@ @clientId@ - identical to #TP3c
** @(PBE2b)@ @action@ - identical to #TP3b - null if @error@ is present
** @(PBE2c)@ @error@ - an @ErrorInfo@ object representing the failure reason for this channel - null if @action@ is present


h3(#options). Option types

h4. ClientOptions
Expand Down Expand Up @@ -1525,6 +1562,7 @@ class Rest:
constructor(ClientOptions) // RSC1
auth: Auth // RSC5
push: Push
batch: BatchOperations // BO1
Copy link
Copy Markdown
Member

@paddybyers paddybyers May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@Peter-Maguire Peter-Maguire May 25, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

device() => io LocalDevice
channels: Channels<RestChannel> // RSN1
request(
Expand Down Expand Up @@ -1697,13 +1735,40 @@ class RealtimeChannel:
unsubscribe(String, (Message) ->) // RTL8a
setOptions(options: ChannelOptions) => io // RTL16

class BatchOperations:
Copy link
Copy Markdown
Member

@paddybyers paddybyers May 11, 2022

Choose a reason for hiding this comment

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

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.

publish([BatchSpec]) => BatchResult<BatchPublishResponse> // BO2a
Copy link
Copy Markdown
Member

@lmars lmars May 18, 2022

Choose a reason for hiding this comment

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

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.

publish(BatchSpec) => BatchResult<BatchPublishResponse> // BO2a
getPresence([String]) => BatchResult<BatchPresenceResponse> // BO2b

class BatchResult<T>:
error: ErrorInfo? // BPA2b
responses: []T? // BPA2a

class BatchPublishResponse:
channel: String // BPB2a
messageId: String? // BPB2b
error: ErrorInfo? // BPB2c

class BatchPresenceResponse:
channel: String // BPD2a
presence: []BatchPresence // PBD2b
Copy link
Copy Markdown
Member

@lmars lmars May 18, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@Peter-Maguire Peter-Maguire May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@lmars lmars May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@Peter-Maguire Peter-Maguire May 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@lmars lmars May 27, 2022

Choose a reason for hiding this comment

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

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
                        }
                ]
        }
]

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022

Choose a reason for hiding this comment

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

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


class BatchPresence:
clientId: string
action: string?
error: ErrorInfo?
Copy link
Copy Markdown
Member

@lmars lmars May 18, 2022

Choose a reason for hiding this comment

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

I think this error belongs on BatchPresenceResponse

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022

Choose a reason for hiding this comment

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

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


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

class BatchSpec:
Copy link
Copy Markdown
Member

@lmars lmars May 18, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@Peter-Maguire Peter-Maguire May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@lmars lmars May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@owenpearson owenpearson Jun 8, 2022

Choose a reason for hiding this comment

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

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

channels: [String]
messages: [Message]

enum ChannelState:
INITIALIZED
ATTACHING
Expand Down