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

Add support for bulk rest publish API #439

Merged
merged 4 commits into from
Dec 13, 2018
Merged

Add support for bulk rest publish API #439

merged 4 commits into from
Dec 13, 2018

Conversation

paddybyers
Copy link
Member

This adds support for the bulk publish API:

POST /messages

This performs a publish in parallel of messages to channels specified in the request body.

A single BatchSpec is an object of the form:

{
  channels: <channels>,
  messages: <messages>
}

where:

  • <channels> is a single channel name String, or an Array of channel name Strings; and
  • <messages> is a single Message, or an Array of Messages.

The bulk publish endpoint accepts a request body containing either a single BatchSpec object, or an array of BatchSpec objects.

Therefore the following are all valid request bodies for a bulk publish request:

{
  channels: ['a channel name, containing a comma', 'another channel name'],
  messages: {data: 'My message contents'}
}
[
  {
    channels: ['a channel name, containing a comma', 'another channel name'],
    messages: {data: 'My message contents'}
  },
  {
    channels: 'single channel',
    messages: [
      {data: 'My message contents'},
      {name: 'an event', data: 'My event message contents'},
    ]
  }
]

There is an obvious mapping from the request body to the array of individual publish requests.

@mattheworiordan
Copy link
Member

Interesting. Is this meant to be a working prototype towards adding this to the spec later? Perhaps in 1.2?

@paddybyers
Copy link
Member Author

Is this meant to be a working prototype towards adding this to the spec later? Perhaps in 1.2?

Yes, that's the question I suppose.

@SimonWoolf
Copy link
Member

Are we sure adding a rest#publish here isn't going to limit our options later? E.g. if we later decide on a spec for batch publishing in 1.2 that differs from this (e.g. say we decide we want more flexibility wrt channelOptions than this implementation allows, i.e. we don't want to assume that every channel in the batchspec will have the same encryption key), is it possible we'll then have a backwards compatibility issue? I suppose that may be less of a problem with java as the method could just be overloaded with a different signature (in that example, one without a ChannelOptions).

We also have early discussions re adding a more general client#publish. Again maybe less of an issue with java as such a method would have a different signature, but for more dynamic languages we may want batch-publish and normal-publish-but-on-the-client methods to have different names, so we don't have to detect whether a passed in array is an array of batchspecs or an array of messages. And then for cross-lib consistency we'd want java to use the same names. Which would imply we might want to call this method something other than publish -- maybe publishBatch? Or do we not think that'll be an issue?

@bbeaudreault
Copy link

Would it be acceptable to mark the API experimental or beta with an annotation? That is a common pattern in other libraries, which leaves space for early access to APIs while also accounting for future changes when it becomes standard.

@paddybyers
Copy link
Member Author

Googling around the consensus seems to be to have an @experimental annotation, but there's no standard definition of one. There's also the option of @deprecated(EXPERIMENTAL) which is a misuse of deprecated but you get compiler/IDE support for it.

@bbeaudreault
Copy link

Annotations are easy to create, I think it's really more about the documentation it provides paired with javadoc and a disclosure in the API docs.

@paddybyers
Copy link
Member Author

Are we sure adding a rest#publish here isn't going to limit our options later?

I don't know tbh but, give that, I'm ok to name it publishBatch(), at least until we've got a decision.

See 98c072f

@paddybyers paddybyers force-pushed the bulk-publish branch 2 times, most recently from fe18cc7 to e82abdd Compare December 12, 2018 17:46
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.

4 participants