You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "vinkal-chudgar (via GitHub)" <gi...@apache.org> on 2025/11/10 21:25:53 UTC
[PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
vinkal-chudgar opened a new pull request, #24965:
URL: https://github.com/apache/pulsar/pull/24965
Fixes #24963
### Motivation
The in-flight deduplication for `BinaryProtoLookupService#getPartitionedTopicMetadata `used only `TopicName`. Calls that differ in `metadataAutoCreationEnabled` or `useFallbackForNonPIP344Brokers` were coalesced, which could return a future that does not match the requested behavior.
**Concrete example:**
- Thread A calls `getPartitionedTopicMetadata(topic, true, false)` and creates a future
- Thread B calls `getPartitionedTopicMetadata(topic, false, true) `and receives the same future from A
- Thread B gets a result that reflects `metadataAutoCreationEnabled=true`, which is incorrect
This PR fixes deduplication by keying in-flight requests on all method parameters.
### Modifications
- Introduced `PartitionedTopicMetadataKey(topicName, metadataAutoCreationEnabled, useFallbackForNonPIP344Brokers)`
- Changed `partitionedMetadataInProgress` to use `PartitionedTopicMetadataKey` as the map key instead of `TopicName`
- Used `computeIfAbsent` with the new key and removed the entry on completion with `remove(key, future)`
- Tests
- Added BinaryProtoLookupServiceTest#testPartitionedMetadataDeduplicationAndCleanup
- Added BinaryProtoLookupServiceTest#testPartitionedMetadataDeduplicationDifferentParameterCombinations
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- Added BinaryProtoLookupServiceTest#testPartitionedMetadataDeduplicationAndCleanup
- Added BinaryProtoLookupServiceTest#testPartitionedMetadataDeduplicationDifferentParameterCombinations
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/vinkal-chudgar/pulsar/pull/5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3516646351
> > > > Why could take the method params into account ? There will be more request to the broker, but the result is the same.
> > >
> > >
> > > @Technoboy- I think that this is more about correctness than the possibility that the broker response might be the same in most cases. It's strictly not the same in all cases. Therefore I think that such bugs should be fixed.
> >
> >
> > This is not a bug
>
> @Technoboy- I think that depends on how you define a bug. The impact of the issue is that `boolean metadataAutoCreationEnabled, boolean useFallbackForNonPIP344Brokers` parameters for the `getPartitionedTopicMetadata` method are ignored for any other requests which are made while there's a request in progress. Let's say that the first request is made with `metadataAutoCreationEnabled=false` and while it's in progress a request with `metadataAutoCreationEnabled=true` is made. This will definitely result in an inconsistent situation. However, I agree that the likelihood that this would happen is low. Regardless of that, such issues should be fixed and I would consider them bugs. Your definition of a bug could vary and I think it's fine.
Make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #24965:
URL: https://github.com/apache/pulsar/pull/24965
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3515191258
> > Why could take the method params into account ? There will be more request to the broker, but the result is the same.
>
> @Technoboy- I think that this is more about correctness than the possibility that the broker response might be the same in most cases. It's strictly not the same in all cases. Therefore I think that such bugs should be fixed.
This is not a bug
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3515180949
> Why could take the method params into account ? There will be more request to the broker, but the result is the same.
@Technoboy- I think that this is more about correctness than the possibility that the broker response might be the same in most cases. It's strictly not the same in all cases. Therefore I think that such bugs should be fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3515217524
> > > Why could take the method params into account ? There will be more request to the broker, but the result is the same.
> >
> >
> > @Technoboy- I think that this is more about correctness than the possibility that the broker response might be the same in most cases. It's strictly not the same in all cases. Therefore I think that such bugs should be fixed.
>
> This is not a bug
I think that depends on how you define a bug. The impact of the issue is that `boolean metadataAutoCreationEnabled, boolean useFallbackForNonPIP344Brokers` parameters for the `getPartitionedTopicMetadata` method are ignored for any other requests which are made while there's a request in progress. Let's say that the first request is made with `metadataAutoCreationEnabled=false` and while it's in progress a request with `metadataAutoCreationEnabled=true` is made. This will definitely result in an inconsistent situation. However, I agree that the likelihood that this would happen is low. Regardless of that, such issues should be fixed and I would consider them bugs. Your definition of a bug could vary and I think it's fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3516846449
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24965?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `61.11111%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 74.30%. Comparing base ([`0896c0a`](https://app.codecov.io/gh/apache/pulsar/commit/0896c0a5fe23c57179faf8752b416bee5e43b6a2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ae68bbd`](https://app.codecov.io/gh/apache/pulsar/commit/ae68bbd0d1ddf211d8567c50856226b42a3d62e8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24965?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...e/pulsar/client/impl/BinaryProtoLookupService.java](https://app.codecov.io/gh/apache/pulsar/pull/24965?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FBinaryProtoLookupService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0JpbmFyeVByb3RvTG9va3VwU2VydmljZS5qYXZh) | 61.11% | [3 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/24965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #24965 +/- ##
============================================
- Coverage 74.44% 74.30% -0.14%
+ Complexity 34071 33520 -551
============================================
Files 1920 1920
Lines 150062 150077 +15
Branches 17404 17408 +4
============================================
- Hits 111708 111515 -193
- Misses 29495 29672 +177
- Partials 8859 8890 +31
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24965/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/24965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.32% <55.55%> (-0.32%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/24965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.81% <55.55%> (-0.09%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.85% <61.11%> (-0.12%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24965?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...e/pulsar/client/impl/BinaryProtoLookupService.java](https://app.codecov.io/gh/apache/pulsar/pull/24965?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FBinaryProtoLookupService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0JpbmFyeVByb3RvTG9va3VwU2VydmljZS5qYXZh) | `79.49% <61.11%> (-1.76%)` | :arrow_down: |
... and [97 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24965/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "vinkal-chudgar (via GitHub)" <gi...@apache.org>.
vinkal-chudgar commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3515428703
> Why could take the method params into account ? There will be more request to the broker, but the result is the same.
@Technoboy- The reason we must include these parameters in the key is that they can lead to fundamentally different outcomes for the caller, breaking the API contract if not handled separately. The logic within `getPartitionedTopicMetadataAsync` demonstrates this.
```
if (!metadataAutoCreationEnabled && !clientCnx.isSupportsGetPartitionedMetadataWithoutAutoCreation()) {
if (useFallbackForNonPIP344Brokers) {
// Fall back to old behavior
finalAutoCreationEnabled = true;
} else {
// This path fails immediately
partitionFuture.completeExceptionally(new PulsarClientException.FeatureNotSupportedException(...));
return;
}
}
// This path sends a request to the broker
ByteBuf request = Commands.newPartitionMetadataRequest(topicName.toString(), requestId, finalAutoCreationEnabled);
```
Here is a practical example of why caching them separately is critical, as two calls with the same `TopicName` but different parameters can produce completely different results:
- **When the broker does not support PIP-344**, the outcome depends entirely on the `useFallbackForNonPIP344Brokers` flag:
* One call might fail immediately with a `FeatureNotSupportedException`.
* Another might fall back to the old behavior and succeed.
If we only deduped by `TopicName`, a caller that expects a failure could incorrectly receive a successful result from the other caller. **This breaks the method contract for that caller.**
- **Even when the broker supports PIP-344**, the `metadataAutoCreationEnabled` flag changes the request sent. Coalescing requests with different values for this flag hides the caller’s chosen behavior and side effects (like topic auto-creation).
Regarding your point about increasing request volume: this change still correctly deduplicates all concurrent calls that use the exact same parameters. A new request is only sent when the parameters are different, ensuring we respect the caller's intent while still preventing redundant traffic for identical calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix deduplication for getPartitionedTopicMetadata to include method parameters [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #24965:
URL: https://github.com/apache/pulsar/pull/24965#issuecomment-3514822783
Why could take the method params into account ? There will be more request to the broker, but the result is the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org