You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/28 19:14:48 UTC

[GitHub] [pulsar] heesung-sn opened a new pull request, #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

heesung-sn opened a new pull request, #18254:
URL: https://github.com/apache/pulsar/pull/18254

   <!--
   ### Contribution Checklist
     
     - PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*. 
   
     - Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
     
     - Each pull request should address only one issue, not mix up code from multiple issues.
     
     - Each commit in the pull request has a meaningful commit message
   
     - Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
   -->
   
   ### Motivation
   The index-based publisher stat aggregation(configured by `aggregatePublisherStatsByProducerName`=false, default) can burst memory and wrongly aggregate publisher metrics if each partition stat returns a different size or order of the publisher list.
   In the worst case, if there are many partitions and publishers created and closed concurrently, the current code can create PublisherStatsImpl objects exponentially, and this can cause a high GC time or OOM.
   
   Issue Code reference:
   
   https://github.com/apache/pulsar/commit/2c428f7fcc740525e8120566c0272fc863ffebf1#diff-02e50674125a597f8ae3405a884590759f2fdaa10104cea511d5ea44b6ff6490R224-R247
   
   <!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
   
   ### Modifications
   
   - Deprecated the `aggregatePublisherStatsByProducerName` broker config because the default, the index-based aggregation is inherently wrong in a highly concurrent producer environment(where the order and size of the publisher stat list are not guaranteed to be the same). The publisher stats need to be aggregated by a unique key, the producer name(aggregatePublisherStatsByProducerName=true).
   - If PublisherStatsImpl.publisherName happens to be null, it will aggregate it with the stat object with "null" name. (However, this is unlikely because the broker generates a globally unique name If not given.)
   - Use HashMap instead List to aggregate publisher stats.
   - Removed sorting from getPublisher(), as this can be expensive for a large number of publishers.
   - Simplified the stat aggregation code.
   
   
   <!-- Describe the modifications you've done. -->
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change added unit tests.
   
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If the box was checked, please highlight the changes*
   
   - [ ] Dependencies (add or upgrade a dependency)
   - [x] The public API
   - [ ] The schema
   - [x] The default values of configurations
   - [ ] The threading model
   - [ ] The binary protocol
   - [ ] The REST endpoints
   - [ ] The admin CLI options
   - [ ] Anything that affects deployment
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [x] `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/heesung-sn/pulsar/pull/13
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
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

[GitHub] [pulsar] equanz commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
equanz commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1314690535

   I have no other suggestions. Why don't you discuss or vote on the dev@ ML before introducing this feature?


-- 
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

[GitHub] [pulsar] heesung-sn commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1314508133

   @equanz,
   
   To fix the issue, do you have any other suggestions other than what I mentioned above?


-- 
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

[GitHub] [pulsar] Technoboy- commented on a diff in pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#discussion_r1010320591


##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/PublisherStatsImpl.java:
##########
@@ -49,8 +49,9 @@ public class PublisherStatsImpl implements PublisherStats {
     /** Id of this publisher. */
     public long producerId;
 
-    /** Whether partial producer is supported at client. */
-    public boolean supportsPartialProducer;
+    /** Whether partial producer is supported at client. supportsPartialProducer is true always. */
+    @Deprecated
+    public boolean supportsPartialProducer = true;

Review Comment:
   Why still keep this ?



-- 
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

[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#discussion_r1010629681


##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/PublisherStats.java:
##########
@@ -44,6 +44,7 @@ public interface PublisherStats {
     long getProducerId();
 
     /** Whether partial producer is supported at client. */
+    @Deprecated
     boolean isSupportsPartialProducer();

Review Comment:
   This is already exposed to the client interface. Are we allowed to make a breaking change?



-- 
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

[GitHub] [pulsar] heesung-sn commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1306501167

   Yes, this pr is not backward-compatible to deprecate the broken feature.


-- 
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

[GitHub] [pulsar] equanz commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
equanz commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1305417873

   In my understanding, old partitioned producer clients are not aggregated by producerName completely. So, this PR introduce breaking changes to them.
   Partitioned producers have the same producerName between partitions as default from https://github.com/apache/pulsar/pull/10279 commit.
   Therefore, I kept a list-indexed aggregation for backward compatibility.
   https://github.com/apache/pulsar/pull/12401
   I think we should not introduce breaking changes, at least between the same major versions.
   
   In addition, I think this approach doesn't *deprecate* a feature but removes a feature.
   Users can't disable `aggregatePublisherStatsByProducerName` from config.
   


-- 
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

[GitHub] [pulsar] heesung-sn commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1312084363

   @equanz,
   
   Thank you for sharing the example. 
   
   Although it is not desirable, I think we could be more lenient on this stat API change(assuming this `publishers` stat struct is used for human admins only for ad-hoc checks).
   
   If this breaking change is not acceptable, I guess we could push this to the next major version. 
   
   Also for the next major version, 
   when there are more than thousands of producers(consumers) for a (partitioned-)topic, it is expensive to aggregate each publisher(subscriptions)'s stats on-the-fly across the brokers. I think we could further define producers(subscriptions)' API like the below and drop the `publishers`  and `subscriptions` structs from `topics (partitioned-)stats` returns.
   ```
   pulsar-admin publishers list my-topic --last-pagination-key xyz
   pulsar-admin publishers stats my-producer
   
   # similarly for subscriptions
   ````
   
   
   
   


-- 
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

[GitHub] [pulsar] equanz commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
equanz commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1311168182

   For example, when enabling `aggregatePublisherStatsByProducerName` in the v2.10.1 server
   
   ```
   % grep 'aggregatePublisherStatsByProducerName' conf/standalone.conf
   aggregatePublisherStatsByProducerName=true
   ```
   
   and create the partitioned producer to the partitioned topic by the v2.8.4 client,
   
   ```
    ./bin/pulsar-perf produce -r 1 -s 10 persistent://public/default/pt
   ...
   11:28:34.053 [main] INFO  org.apache.pulsar.testclient.PerformanceProducer - Starting Pulsar perf producer with config: {
   ...
     "producerName" : null,
   ...
   11:28:34.969 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ProducerImpl - [persistent://public/default/pt-partition-0] [standalone-0-1] Created producer on cnx [id: 0x0e5ab2e4, L:/127.0.0.1:49990 - R:localhost/127.0.0.1:6650]
   11:28:34.992 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ProducerImpl - [persistent://public/default/pt-partition-1] [standalone-0-0] Created producer on cnx [id: 0x32a3290d, L:/127.0.0.1:49989 - R:localhost/127.0.0.1:6650]
   ...
   ```
   
   then the broker returns partitioned topic stats like the one below.
   
   ```
   % ./bin/pulsar-admin topics partitioned-stats persistent://public/default/pt
   ...
     "publishers" : [ {
       "msgRateIn" : 0.0,
       "msgThroughputIn" : 0.0,
       "averageMsgSize" : 0.0,
       "chunkedMessageRate" : 0.0,
       "producerId" : 0,
       "supportsPartialProducer" : false
     } ],
   ...
   ```
   
   However, when running with your fixes on the server side and creating a producer to a partitioned topic by the v2.8.4 client,
   
   ```
   % ./bin/pulsar-perf produce -r 1 -s 10 persistent://public/default/pt
   ...
     "producerName" : null,
   ...
   11:30:37.289 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ProducerImpl - [persistent://public/default/pt-partition-1] [standalone-0-1] Created producer on cnx [id: 0x393b0652, L:/127.0.0.1:50035 - R:localhost/127.0.0.1:6650]
   11:30:37.307 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ProducerImpl - [persistent://public/default/pt-partition-0] [standalone-0-0] Created producer on cnx [id: 0x9b73aa95, L:/127.0.0.1:50034 - R:localhost/127.0.0.1:6650]
   ...
   ```
   
   then the broker returns partitioned topic stats like the one below. As you can see, publishers can't be aggregated by producerName even if the partitioned producer is alone in the partitioned topic. It is breaking changes. Is it allowed on this project?
   
   ```
   % ./bin/pulsar-admin topics partitioned-stats persistent://public/default/pt
   ...
     "publishers" : [ {
       "msgRateIn" : 0.0,
       "msgThroughputIn" : 0.0,
       "averageMsgSize" : 0.0,
       "chunkedMessageRate" : 0.0,
       "producerId" : 0,
       "supportsPartialProducer" : true,
       "producerName" : "standalone-0-1"
     }, {
       "msgRateIn" : 0.0,
       "msgThroughputIn" : 0.0,
       "averageMsgSize" : 0.0,
       "chunkedMessageRate" : 0.0,
       "producerId" : 0,
       "supportsPartialProducer" : true,
       "producerName" : "standalone-0-0"
     } ],
   ...
   ```


-- 
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

[GitHub] [pulsar] codecov-commenter commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1298401502

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/18254?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18254](https://codecov.io/gh/apache/pulsar/pull/18254?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6228a5c) into [master](https://codecov.io/gh/apache/pulsar/commit/6c65ca0d8a80bfaaa4d5869e0cea485f5c94369b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c65ca0) will **increase** coverage by `3.96%`.
   > The diff coverage is `35.90%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/18254/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/18254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #18254      +/-   ##
   ============================================
   + Coverage     34.91%   38.88%   +3.96%     
   - Complexity     5707     8307    +2600     
   ============================================
     Files           607      685      +78     
     Lines         53396    67330   +13934     
     Branches       5712     7215    +1503     
   ============================================
   + Hits          18644    26180    +7536     
   - Misses        32119    38132    +6013     
   - Partials       2633     3018     +385     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `38.88% <35.90%> (+3.96%)` | :arrow_up: |
   
   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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...org/apache/bookkeeper/mledger/LedgerOffloader.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9MZWRnZXJPZmZsb2FkZXIuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...che/bookkeeper/mledger/LedgerOffloaderFactory.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9MZWRnZXJPZmZsb2FkZXJGYWN0b3J5LmphdmE=) | `0.00% <ø> (ø)` | |
   | [...pache/bookkeeper/mledger/LedgerOffloaderStats.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9MZWRnZXJPZmZsb2FkZXJTdGF0cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...ookkeeper/mledger/LedgerOffloaderStatsDisable.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9MZWRnZXJPZmZsb2FkZXJTdGF0c0Rpc2FibGUuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...a/org/apache/bookkeeper/mledger/ManagedCursor.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9NYW5hZ2VkQ3Vyc29yLmphdmE=) | `85.71% <ø> (ø)` | |
   | [...che/bookkeeper/mledger/ManagedLedgerException.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9NYW5hZ2VkTGVkZ2VyRXhjZXB0aW9uLmphdmE=) | `41.17% <ø> (ø)` | |
   | [...bookkeeper/mledger/ManagedLedgerFactoryConfig.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9NYW5hZ2VkTGVkZ2VyRmFjdG9yeUNvbmZpZy5qYXZh) | `86.66% <ø> (ø)` | |
   | [...g/apache/bookkeeper/mledger/ManagedLedgerInfo.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9NYW5hZ2VkTGVkZ2VySW5mby5qYXZh) | `22.22% <ø> (ø)` | |
   | [...he/bookkeeper/mledger/OffloadedLedgerMetadata.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9PZmZsb2FkZWRMZWRnZXJNZXRhZGF0YS5qYXZh) | `0.00% <ø> (ø)` | |
   | [...ava/org/apache/bookkeeper/mledger/ScanOutcome.java](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9TY2FuT3V0Y29tZS5qYXZh) | `0.00% <ø> (ø)` | |
   | ... and [729 more](https://codecov.io/gh/apache/pulsar/pull/18254/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
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

[GitHub] [pulsar] poorbarcode commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1299561284

   /pulsarbot rerun-failure-checks


-- 
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

[GitHub] [pulsar] heesung-sn closed pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn closed pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config
URL: https://github.com/apache/pulsar/pull/18254


-- 
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

[GitHub] [pulsar] Technoboy- closed pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config
URL: https://github.com/apache/pulsar/pull/18254


-- 
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

[GitHub] [pulsar] Technoboy- commented on a diff in pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#discussion_r1010320911


##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/PublisherStats.java:
##########
@@ -44,6 +44,7 @@ public interface PublisherStats {
     long getProducerId();
 
     /** Whether partial producer is supported at client. */
+    @Deprecated
     boolean isSupportsPartialProducer();

Review Comment:
   Why not remove this ?



-- 
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

[GitHub] [pulsar] heesung-sn commented on pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#issuecomment-1314736755

   Opened a pulsar community discussion thread here.
   
   https://lists.apache.org/thread/vofv1oz0wvzlwk4x9vk067rhkscn8bqo


-- 
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

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#discussion_r1016166458


##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/PublisherStats.java:
##########
@@ -44,6 +44,7 @@ public interface PublisherStats {
     long getProducerId();
 
     /** Whether partial producer is supported at client. */
+    @Deprecated
     boolean isSupportsPartialProducer();

Review Comment:
   I think this is the correct change. If it has already been released, we cannot remove it without first deprecating it. I don't know that we have documented guidance on how long to keep methods like this marked as deprecated.



-- 
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

[GitHub] [pulsar] heesung-sn commented on a diff in pull request #18254: [fix][broker] Deprecated aggregatePublisherStatsByProducerName config

Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #18254:
URL: https://github.com/apache/pulsar/pull/18254#discussion_r1010630808


##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/PublisherStatsImpl.java:
##########
@@ -49,8 +49,9 @@ public class PublisherStatsImpl implements PublisherStats {
     /** Id of this publisher. */
     public long producerId;
 
-    /** Whether partial producer is supported at client. */
-    public boolean supportsPartialProducer;
+    /** Whether partial producer is supported at client. supportsPartialProducer is true always. */
+    @Deprecated
+    public boolean supportsPartialProducer = true;

Review Comment:
   The getter of this member var is already exposed to the client interface. Are we allowed to make a breaking change?



-- 
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