You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "backuitist (via GitHub)" <gi...@apache.org> on 2025/12/16 07:58:25 UTC
[PR] Pulsar consumer does not need to query for partitions [pinot]
backuitist opened a new pull request, #17379:
URL: https://github.com/apache/pinot/pull/17379
(no comment)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Pulsar consumer does not need to query for partitions [pinot]
Posted by "backuitist (via GitHub)" <gi...@apache.org>.
backuitist commented on code in PR #17379:
URL: https://github.com/apache/pinot/pull/17379#discussion_r2632515070
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarPartitionLevelConsumer.java:
##########
@@ -46,8 +47,8 @@ public PulsarPartitionLevelConsumer(String clientId, StreamConfig streamConfig,
super(clientId, streamConfig);
String topicName = _config.getPulsarTopicName();
try {
- List<String> partitions = _pulsarClient.getPartitionsForTopic(topicName).get();
- _reader = _pulsarClient.newReader().topic(partitions.get(partition)).startMessageId(MessageId.earliest)
+ String partitionName = TopicName.getTopicPartitionNameString(topicName, partition);
Review Comment:
In what way did it validate the existence of these topics? Do you mean that `partitions.get(partition)` would throw an IndexOutOfBoundException?
If anything this implementation should give you a better error message in this (unlikely) event.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Pulsar consumer does not need to query for partitions [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #17379:
URL: https://github.com/apache/pinot/pull/17379#discussion_r2634132765
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarPartitionLevelConsumer.java:
##########
@@ -46,8 +47,8 @@ public PulsarPartitionLevelConsumer(String clientId, StreamConfig streamConfig,
super(clientId, streamConfig);
String topicName = _config.getPulsarTopicName();
try {
- List<String> partitions = _pulsarClient.getPartitionsForTopic(topicName).get();
- _reader = _pulsarClient.newReader().topic(partitions.get(partition)).startMessageId(MessageId.earliest)
+ String partitionName = TopicName.getTopicPartitionNameString(topicName, partition);
Review Comment:
> In what way did it validate the existence of these topics?
Talking about _pulsarClient.getPartitionsForTopic(topicName) throwng PulsarClientException.TopicDoesNotExistException for invalid topic.
But I believe in the new implementation the validation is deferred to `Reader.create()`, so its 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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Pulsar consumer does not need to query for partitions [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17379:
URL: https://github.com/apache/pinot/pull/17379
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Pulsar consumer does not need to query for partitions [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17379:
URL: https://github.com/apache/pinot/pull/17379#issuecomment-3662366574
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17379?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.60%. Comparing base ([`d47ea27`](https://app.codecov.io/gh/apache/pinot/commit/d47ea27b4ee724ea4b52aa07db2ef22cfd5071fd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`00a4df0`](https://app.codecov.io/gh/apache/pinot/commit/00a4df06a2ed14eec864e3d97cd5158dc72c5a7f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (d47ea27) and HEAD (00a4df0). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (d47ea27) | HEAD (00a4df0) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17379 +/- ##
============================================
- Coverage 63.22% 55.60% -7.62%
+ Complexity 1475 702 -773
============================================
Files 3147 2459 -688
Lines 187575 138856 -48719
Branches 28712 22139 -6573
============================================
- Hits 118590 77213 -41377
+ Misses 59794 55128 -4666
+ Partials 9191 6515 -2676
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <ø> (-7.58%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <ø> (-7.62%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <ø> (-7.62%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <ø> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17379/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17379?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Pulsar consumer does not need to query for partitions [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #17379:
URL: https://github.com/apache/pinot/pull/17379#discussion_r2632024458
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarPartitionLevelConsumer.java:
##########
@@ -46,8 +47,8 @@ public PulsarPartitionLevelConsumer(String clientId, StreamConfig streamConfig,
super(clientId, streamConfig);
String topicName = _config.getPulsarTopicName();
try {
- List<String> partitions = _pulsarClient.getPartitionsForTopic(topicName).get();
- _reader = _pulsarClient.newReader().topic(partitions.get(partition)).startMessageId(MessageId.earliest)
+ String partitionName = TopicName.getTopicPartitionNameString(topicName, partition);
Review Comment:
Prev implementation was implicitly validating that topic and partition exists.
This implementation will not but I believe if partitionName is invalid the reader will fail and throw exception below right? Can you confirm?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org