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

[Java Client] Send CloseProducer on timeout #13161

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

michaeljmarshall
Copy link
Member

Motivation

When producer creation times out, send the CloseProducer before sending a command to recreate the Producer. This ensures a better time to recovery.

This issue was discussed on the mailing list here, https://lists.apache.org/thread/tko0z4jg0oq0yf931rbow2zf9fq8wjt1. The protocol spec has already been updated to indicate that this is the correct client behavior #12948.

Modifications

  • Update the java client to send CloseProducer when the producer creation times out.
  • Add tests.
  • Clean up some comments and unnecessary calls to close() in the modified test file.

Verifying this change

I added tests to verify this change.

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

There are no breaking changes. This is an improvement to the protocol. The previous implementation was valid, but not ideal.

Documentation

@michaeljmarshall michaeljmarshall added this to the 2.10.0 milestone Dec 7, 2021
@michaeljmarshall michaeljmarshall self-assigned this Dec 7, 2021
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 7, 2021
@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

2 similar comments
@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeljmarshall
Copy link
Member Author

Closing and reopening to get the fix to the BrokerServiceLookupTest, a flaky test.

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

@merlimat merlimat added release/2.8.3 release/2.9.1 type/bug The PR fixed a bug or issue reported a bug labels Dec 9, 2021
@merlimat merlimat merged commit 27d5429 into apache:master Dec 9, 2021
@michaeljmarshall michaeljmarshall deleted the close-producer branch December 9, 2021 19:18
lhotari pushed a commit that referenced this pull request Dec 10, 2021
* [Java Client] Send CloseProducer on timeout

* Fix failed test from ClientErrorsTest

(cherry picked from commit 27d5429)
@lhotari lhotari added cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.2 and removed release/2.8.3 labels Dec 10, 2021
lhotari pushed a commit that referenced this pull request Dec 10, 2021
* [Java Client] Send CloseProducer on timeout

* Fix failed test from ClientErrorsTest

(cherry picked from commit 27d5429)
@lhotari lhotari added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 10, 2021
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 10, 2021
* [Java Client] Send CloseProducer on timeout

* Fix failed test from ClientErrorsTest

(cherry picked from commit 27d5429)
(cherry picked from commit b72d452)
codelipenghui pushed a commit that referenced this pull request Dec 11, 2021
* [Java Client] Send CloseProducer on timeout

* Fix failed test from ClientErrorsTest

(cherry picked from commit 27d5429)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Dec 11, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
* [Java Client] Send CloseProducer on timeout

* Fix failed test from ClientErrorsTest
RobertIndie pushed a commit to apache/pulsar-client-go that referenced this pull request Jul 20, 2023
### Motivation

This change is the same as apache/pulsar#13161 and apache/pulsar#16616, and is justified by these lines of our binary protocol spec:

* https://github.com/apache/pulsar-site/blob/9b4b3d39014bd47c0bb9f66742b89bcb40ed7f07/docs/developing-binary-protocol.md?plain=1#L301-L304
* https://github.com/apache/pulsar-site/blob/9b4b3d39014bd47c0bb9f66742b89bcb40ed7f07/docs/developing-binary-protocol.md?plain=1#L468-L471

### Modifications

* When a producer or a consumer times out during creation, make an attempt to close the producer or consumer by sending the appropriate close command. Failures can safely be ignored because the only time that the close will actually matter is when the TCP connection is open for other protocol messages. The one nuance is that we send the close command to the same address pair that we send the create command.
RobertIndie pushed a commit to apache/pulsar-client-go that referenced this pull request Sep 7, 2023
### Motivation

This change is the same as apache/pulsar#13161 and apache/pulsar#16616, and is justified by these lines of our binary protocol spec:

* https://github.com/apache/pulsar-site/blob/9b4b3d39014bd47c0bb9f66742b89bcb40ed7f07/docs/developing-binary-protocol.md?plain=1#L301-L304
* https://github.com/apache/pulsar-site/blob/9b4b3d39014bd47c0bb9f66742b89bcb40ed7f07/docs/developing-binary-protocol.md?plain=1#L468-L471

### Modifications

* When a producer or a consumer times out during creation, make an attempt to close the producer or consumer by sending the appropriate close command. Failures can safely be ignored because the only time that the close will actually matter is when the TCP connection is open for other protocol messages. The one nuance is that we send the close command to the same address pair that we send the create command.

(cherry picked from commit d4e08c6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.7.4 release/2.8.2 release/2.9.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants