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 sendMessages and flush to StreamingRequestClientCall #538

Merged
merged 6 commits into from
Aug 5, 2019

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Aug 2, 2019

Motivation:

The API for StreamingRequestClientCall does not make it easy for users
to send a batch of messages and then flush at the end.

Modifications:

Add methods to StreamingRequestClientCall to:

  • send messages, and
  • manually flush.

Result:

Users can send a batch of messages and manually flush for calls which
stream from the client.

Motivation:

The API for `StreamingRequestClientCall` does not make it easy for users
to send a batch of messages and then flush at the end.

Modifications:

Add methods to `StreamingRequestClientCall` to:
- send messages, and
- manually flush.

Result:

Users can send a batch of messages and manually flush for calls which
stream from the client.
@glbrntt
Copy link
Collaborator Author

glbrntt commented Aug 2, 2019

Resolves #537

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Should we have the same methods on the server as well?

Sources/GRPC/ClientCalls/ClientCall.swift Outdated Show resolved Hide resolved
Sources/GRPC/ClientCalls/ClientCall.swift Outdated Show resolved Hide resolved
Sources/GRPC/ClientCalls/ClientCall.swift Show resolved Hide resolved
Tests/GRPCTests/StreamingRequestClientCallTests.swift Outdated Show resolved Hide resolved
@glbrntt
Copy link
Collaborator Author

glbrntt commented Aug 2, 2019

Should we have the same methods on the server as well?

Potentially, but I think it would come with a broader set of changes: at the moment there's really no control for the user in how they send messages, only sendResponse(message:) -> EventLoopFuture<Void>.

I'll file another issue for this.

@MrMage
Copy link
Collaborator

MrMage commented Aug 5, 2019

Thinking about this more, what's the benefit of the extra flush: argument over calling flush()? Sorry if I missed that before.

I'm a bit worried that having both future-returning and promise-taking methods that both have a flush: argument might start to get a bit confusing and violate the single responsibility of just sending a message.

@MrMage
Copy link
Collaborator

MrMage commented Aug 5, 2019

LGTM, save for my question about flush: before.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Aug 5, 2019

Hmm I see your point.

We should keep the future/promise APIs. But I think we can do away with flush() and flush: and just always flush in sendMessage/sendMessages.

I think it's a reasonable assumption to make that most users will send a single message at a time in which case I think they'd (probably) want to flush as well. If they don't want to flush when writing a message then they can keep their own buffer of messages and then send them with sendMessages.

The API would then be something like:

  • sendMessage(_ message: RequestMessage) -> EventLoopFuture<Void>
  • sendMessage(_ message: RequestMessage, promise: EventLoopPromise<Void>?)
  • sendMessages<S: Sequence>(_ messages: S) -> EventLoopFuture<Void> where S.Element == RequestMessage
  • sendMessages<S: Sequence>(_ messages: S, promise: EventLoopPromise<Void>?) where S.Element == RequestMessage

What do you think?

@MrMage
Copy link
Collaborator

MrMage commented Aug 5, 2019

We should keep the future/promise APIs. But I think we can do away with flush() and flush: and just always flush in sendMessage/sendMessages.

Agreed.

The API would then be something like:

Looks good to me :-)

@MrMage MrMage closed this Aug 5, 2019
@MrMage MrMage reopened this Aug 5, 2019
Co-Authored-By: Daniel Alm <Daniel.Alm@ForumD.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants