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

[client] Cleanup consumer on multitopic subscribe failure #9419

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

addisonj
Copy link
Contributor

@addisonj addisonj commented Feb 2, 2021

Motivation

Currently, when a multi-topic subscribe fails (via a set of topics or a
regex) we can leave consumers connected, as the multitopic consumer
doesn't close any of the topics.

This means we rely on the client to call closeAsync, otherwise, the
consumer is left in partially open state.

Modifications

This fix changes that, and ensures we call close in the case of an
exception

Verifying this change

This change added tests that ensure that the expected methods are called in the event of subscribe failure

Currently, when a multi-topic subscribe fails (via a set of topics or a
regex) we can leave consumers connected, as the multitopic consumer
doesn't close any of the topics.

This means we rely on the client to call closeAsync, otherwise, the
consumer is left in partially open state.

This fix changes that, and ensures we call close in the case of an
exception
@codelipenghui codelipenghui added release/2.7.1 type/bug The PR fixed a bug or issue reported a bug labels Feb 2, 2021
@codelipenghui codelipenghui added this to the 2.8.0 milestone Feb 2, 2021
@zymap
Copy link
Member

zymap commented Feb 3, 2021

Failing tests record:

integration sql:

Error:  Tests run: 6, Failures: 2, Errors: 0, Skipped: 2, Time elapsed: 349.226 s <<< FAILURE! - in TestSuite
Error:  pulsar-sql-test-suite(org.apache.pulsar.tests.integration.presto.TestPrestoQueryTieredStorage)  Time elapsed: 29.664 s  <<< FAILURE!
org.apache.pulsar.tests.integration.docker.ContainerExecException: /bin/bash -c /pulsar/bin/pulsar sql --execute 'select * from pulsar."presto/ts"."stocks_ts_nons_yjxio" order by entryid;' failed on 3fa284a519c77b9a14f64469696bdbac3f79644c4c0d96913462773fefabc3da with error code 1
	at org.apache.pulsar.tests.integration.utils.DockerUtils$2.onComplete(DockerUtils.java:259)
	at org.testcontainers.shaded.com.github.dockerjava.core.exec.AbstrAsyncDockerCmdExec$1.onComplete(AbstrAsyncDockerCmdExec.java:51)
	at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:276)
	at java.lang.Thread.run(Thread.java:748)

Error:  pulsar-sql-test-suite(org.apache.pulsar.tests.integration.presto.TestPrestoQueryTieredStorage)  Time elapsed: 36.393 s  <<< FAILURE!
org.apache.pulsar.tests.integration.docker.ContainerExecException: /bin/bash -c /pulsar/bin/pulsar sql --execute 'select * from pulsar."presto/ts"."stocks_ts_ns_xbnhw" order by entryid;' failed on 3fa284a519c77b9a14f64469696bdbac3f79644c4c0d96913462773fefabc3da with error code 1
	at org.apache.pulsar.tests.integration.utils.DockerUtils$2.onComplete(DockerUtils.java:259)
	at org.testcontainers.shaded.com.github.dockerjava.core.exec.AbstrAsyncDockerCmdExec$1.onComplete(AbstrAsyncDockerCmdExec.java:51)
	at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:276)
	at java.lang.Thread.run(Thread.java:748)

[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:  org.apache.pulsar.tests.integration.presto.TestPrestoQueryTieredStorage.pulsar-sql-test-suite(org.apache.pulsar.tests.integration.presto.TestPrestoQueryTieredStorage)
[INFO]   Run 1: PASS
Error:    Run 2: TestPrestoQueryTieredStorage.testQueryTieredStorage1 » ContainerExec /bin/bash...
[INFO]   Run 3: PASS
Error:    Run 4: TestPrestoQueryTieredStorage.testQueryTieredStorage2 » ContainerExec /bin/bash...

broker group 1:

Error:  Tests run: 43, Failures: 1, Errors: 0, Skipped: 2, Time elapsed: 1,222.846 s <<< FAILURE! - in org.apache.pulsar.broker.admin.TopicPoliciesTest
Error:  setup(org.apache.pulsar.broker.admin.TopicPoliciesTest)  Time elapsed: 942.58 s  <<< FAILURE!
java.lang.OutOfMemoryError: Java heap space

[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    TopicPoliciesTest.setup ? OutOfMemory Java heap space
[INFO] 
Error:  Tests run: 507, Failures: 1, Errors: 0, Skipped: 1

@zymap
Copy link
Member

zymap commented Feb 3, 2021

/pulsarbot run-failure-checks

@merlimat merlimat merged commit cbe9816 into apache:master Feb 12, 2021
@sijie sijie deleted the cleanup_failed_subscribe branch February 15, 2021 05:13
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Feb 18, 2021
codelipenghui pushed a commit that referenced this pull request Feb 18, 2021
Currently, when a multi-topic subscribe fails (via a set of topics or a
regex) we can leave consumers connected, as the multitopic consumer
doesn't close any of the topics.

This means we rely on the client to call closeAsync, otherwise, the
consumer is left in partially open state.

This fix changes that, and ensures we call close in the case of an
exception

Co-authored-by: Sijie Guo <sijie@apache.org>
(cherry picked from commit cbe9816)
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 release/2.7.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

6 participants