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

[Producer] respect context cancellation in Flush #1165

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

jayshrivastava
Copy link
Contributor

@jayshrivastava jayshrivastava commented Jan 31, 2024

Motivation

The producer's Flush method does not respect context cancellation. If the caller's context get's cancelled, it will have to wait for the producer to finish flushing.

Modifications

This change adds a FlushWithCtx method which takes a context and selects on two channels.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests. See code changes for relevant tests.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable
  • If a feature is not applicable for documentation, explain why? It's a very trivial change. It adds a common pattern in Go.
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@jayshrivastava jayshrivastava changed the title [pulsar] respect context cancellation in producer [Producer] respect context cancellation in producer Jan 31, 2024
@jayshrivastava jayshrivastava marked this pull request as draft January 31, 2024 19:24
@jayshrivastava jayshrivastava marked this pull request as ready for review January 31, 2024 19:24
@jayshrivastava
Copy link
Contributor Author

@RobertIndie Would you mind taking a look at this please?

@@ -239,7 +239,7 @@ type Producer interface {

// Flush all the messages buffered in the client and wait until all messages have been successfully
// persisted.
Flush() error
Flush(ctx context.Context) error
Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that we should have taking the context in this method, right now this would be a breaking API change.

I'd say to add a new FlushWithContext() method and, perhaps, mark the other one as deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed an update. Lmk how it looks!

Btw, do you know if the library will support shutting down via context cancellation in the future? There's some examples of where we don't do it:

func (p *partitionProducer) runEventsLoop() {

func (p *partitionProducer) Close() {
if p.getProducerState() != producerReady {
// Producer is closing
return
}
cp := &closeProducer{doneCh: make(chan struct{})}
p.cmdChan <- cp
// wait for close producer request to complete
<-cp.doneCh
}

As long as things don't take too long to shut down, I suppose it's fine. Ideally, if the context is cancelled, you don't want to wait for a clean/graceful shutdown where we wait for the done channel. We want all the goroutines to terminate right where they are and immediately returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be happy to file an issue if there is not one already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't implement CloseWithCtx such as in this PR because goroutines might leak (the call to close would terminate before the signal is given on the doneCh, indicating that things have shut down). I think it would be a bigger change.

@RobertIndie RobertIndie added this to the v0.13.0 milestone Feb 1, 2024
@jayshrivastava jayshrivastava changed the title [Producer] respect context cancellation in producer [Producer] respect context cancellation in Flush Feb 1, 2024
@RobertIndie RobertIndie merged commit 2a28e21 into apache:master Feb 2, 2024
6 checks passed
RobertIndie pushed a commit that referenced this pull request Feb 29, 2024
### Motivation

The producer's `Flush` method does not respect context cancellation. If the caller's context get's cancelled, it will have to wait for the producer to finish flushing.

### Modifications

This change adds a `FlushWithCtx` method which takes a context and selects on two channels.

(cherry picked from commit 2a28e21)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants