Skip to content

Fix ConsumerBuilderImpl#subscribeAsync blocks calling thread. - #14433

Merged
codelipenghui merged 6 commits into
apache:masterfrom
Technoboy-:fix-14413
Feb 25, 2022
Merged

Fix ConsumerBuilderImpl#subscribeAsync blocks calling thread.#14433
codelipenghui merged 6 commits into
apache:masterfrom
Technoboy-:fix-14413

Conversation

@Technoboy-

Copy link
Copy Markdown
Contributor

Fixes #14413

Motivation

When Retry topics are enabled, ConsumerBuilderImpl performs a backwards-compatibility check to look for DLQ or Retry topics that were created on previous version of Pulsar using a different naming scheme.

It does this by calling getPartitionedTopicMetadata and then using .get() to block while waiting for the results

if (client.getPartitionedTopicMetadata(oldRetryLetterTopic)
        .get(client.conf.getOperationTimeoutMs(), TimeUnit.MILLISECONDS).partitions > 0) {
    retryLetterTopic = oldRetryLetterTopic;
}
if (client.getPartitionedTopicMetadata(oldDeadLetterTopic)
        .get(client.conf.getOperationTimeoutMs(), TimeUnit.MILLISECONDS).partitions > 0) {
    deadLetterTopic = oldDeadLetterTopic;
}

This was implemented in #10129
A partial fix to add a Timeout was implemented in #11597

However this fix does still block the calling thread during the lookup.

This can be an issue for code that attempt to call subscribeAsync on a non-blocking Pool (such as when using Netty). The signature of subscribeAsync implies that it's non-blocking. (And it seems somewhat pointless to have subscribeAsync if it blocks anyway)

Note that this is an undocumented breaking change in behavior. Up until 2.9, it was safe to call this from a non-blocking pool.

In our case, we were running a custom SLF4J log exporter on the same thread pool, which resulted in a deadlock when the number of concurrent subscribeAsync calls exceeded the pool size. (This is probably an extreme example and a poor decision on our part, but perhaps a good example of why unexpected blocking can be dangerous)

Note that blocking call is still made even if the retry and DLQ names are explicitly specified in DeadLetterPolicy. In that scenario the check should not be needed. Aside from blocking the calling Thread, this also results in unneeded lookup requests.

Modifications

  • Make retry and DLQ metadata async.

Documentation

  • no-need-doc

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Feb 23, 2022

@Jason918 Jason918 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall LGTM.
I wonder if it's possible that we can add some kind of client version check to avoid this compatibility check?

@jimmydivvy

Copy link
Copy Markdown

I wonder if it's possible that we can add some kind of client version check to avoid this compatibility check?

Perhaps a flag on DeadLetterPolicy that checks for legacy topics. Enabled by default for compatibility, but can be disabled to avoid the performance overhead?

DeadLetterPolicy.builder().checkForLegacyTopics(false)

@Technoboy-

Technoboy- commented Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

I wonder if it's possible that we can add some kind of client version check to avoid this compatibility check?

Perhaps a flag on DeadLetterPolicy that checks for legacy topics. Enabled by default for compatibility, but can be disabled to avoid the performance overhead?

DeadLetterPolicy.builder().checkForLegacyTopics(false)

Yes, we can open a new issue for discussion.

@Technoboy-

Copy link
Copy Markdown
Contributor Author

@Jason918 @codelipenghui @gaoran10 @mattisonchao
Could you please help review again?

@codelipenghui
codelipenghui merged commit 7a58aeb into apache:master Feb 25, 2022
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.10.0 Feb 25, 2022
codelipenghui pushed a commit that referenced this pull request Feb 25, 2022
gaoran10 pushed a commit that referenced this pull request Mar 1, 2022
@gaoran10 gaoran10 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Mar 2, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Mar 9, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@Technoboy-
Technoboy- deleted the fix-14413 branch August 10, 2022 05:53
@BewareMyPower BewareMyPower removed cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.4 labels Sep 14, 2022
@github-actions github-actions Bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 14, 2022
@github-actions

Copy link
Copy Markdown

@Technoboy- Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

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.

ConsumerBuilderImpl.subscribeAsync blocks calling thread

7 participants