You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/07/01 22:25:48 UTC

[PR] Add config to set consumption rate limit at partition level [pinot]

Jackie-Jiang opened a new pull request, #18904:
URL: https://github.com/apache/pinot/pull/18904

## Summary

Add a new stream config `partition.consumption.rate.limit` to set the realtime consumption rate limit directly at partition level. Unlike the existing `topic.consumption.rate.limit` (which is divided by the topic's partition count), the partition level limit doesn't need to be reconfigured when the partition count changes.

- `RealtimeConsumptionRateManager.createRateLimiter` checks the partition level limit first and uses it directly, skipping the partition count fetch (and its cache) entirely. When both are specified, partition level limit takes precedence. Topic level behavior is unchanged.
- Both rate limit getters in `StreamConfig` now return primitive `double` (non-positive means not throttled) instead of `Optional<Double>`, consistent with other configs. `CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED` is made public.
- Cleanup: removed dead `_groupId` / `getGroupId()` from `StreamConfig` and the unused `GROUP_ID` (`hlc.group.id`, HLC legacy) and `PARTITION_MSG_OFFSET_FACTORY_CLASS` keys from `StreamConfigProperties`; reordered `equals` / `hashCode` / `toString` to match field declaration order and fixed two malformed `toString` labels (missing `=`, `_offSet` typo).

Note: `getTopicConsumptionRateLimit()` signature change and the removed dead members are binary-incompatible for third-party plugins compiled against `pinot-spi`. There are no callers in the Pinot codebase.

--
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #18904:
URL: https://github.com/apache/pinot/pull/18904#issuecomment-4860813645

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18904?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 `90.62500%` with `3 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 56.99%. Comparing base ([`91be61f`](https://app.codecov.io/gh/apache/pinot/commit/91be61f0f4f6c90dd11722f005af43b2a145cb76?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dbe9437`](https://app.codecov.io/gh/apache/pinot/commit/dbe94372905d56b6c01dd0e6269a4b3959504970?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/pinot/pull/18904?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...java/org/apache/pinot/spi/stream/StreamConfig.java](https://app.codecov.io/gh/apache/pinot/pull/18904?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fstream%2FStreamConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbUNvbmZpZy5qYXZh) | 87.50% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18904?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (91be61f) and HEAD (dbe9437). Click for more details.
   > 
   > <details><summary>HEAD has 16 uploads less than BASE</summary>
   >
   >| Flag | BASE (91be61f) | HEAD (dbe9437) |
   >|------|------|------|
   >|java-21|5|1|
   >|unittests|2|1|
   >|temurin|5|1|
   >|unittests2|1|0|
   >|integration|3|0|
   >|integration1|1|0|
   >|custom-integration1|1|0|
   >|integration2|1|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #18904      +/-   ##
   ============================================
   - Coverage     64.80%   56.99%   -7.81%     
   + Complexity     1347        1    -1346     
   ============================================
     Files          3393     2604     -789     
     Lines        211666   152957   -58709     
     Branches      33305    24884    -8421     
   ============================================
   - Hits         137164    87183   -49981     
   + Misses        63431    58352    -5079     
   + Partials      11071     7422    -3649     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/18904/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/18904/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/18904/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/18904/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/18904/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/18904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <90.62%> (-7.81%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/18904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <90.62%> (-7.81%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/18904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <90.62%> (-7.81%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <90.62%> (+<0.01%)` | :arrow_up: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18904/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 Harness](https://app.codecov.io/gh/apache/pinot/pull/18904?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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang commented on code in PR #18904:
URL: https://github.com/apache/pinot/pull/18904#discussion_r3509496245


##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/RealtimeConsumptionRateManagerTest.java:
##########
@@ -91,22 +96,30 @@ public class RealtimeConsumptionRateManagerTest {
   @Test
   public void testCreateRateLimiter() {
     // topic A
-    ConsumptionRateLimiter rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_A, TABLE_NAME);
-    assertEquals(5.0, ((PartitionRateLimiter) rateLimiter).getRate(), DELTA);
+    ConsumptionRateLimiter rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_A, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 5.0, DELTA);
 
     // topic B
-    rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_B, TABLE_NAME);
-    assertEquals(2.5, ((PartitionRateLimiter) rateLimiter).getRate(), DELTA);
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_B, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 2.5, DELTA);
 
     // topic C
-    rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_C, TABLE_NAME);
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_C, TABLE_NAME);
     assertEquals(rateLimiter, NOOP_RATE_LIMITER);
+
+    // topic D: partition level rate limit is used directly, without fetching the partition count
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_D, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 4.0, DELTA);
+
+    // topic E: partition level rate limit takes precedence over topic level rate limit
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_E, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 4.0, DELTA);

Review Comment:
   The test already enforces this: the partition count cache mock is not stubbed for these stream configs, so any partition count lookup would return null and fail the test with an NPE on unboxing. An explicit `verify` would be redundant.



-- 
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #18904:
URL: https://github.com/apache/pinot/pull/18904#discussion_r3509414371


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -198,11 +196,13 @@ public StreamConfig(String tableNameWithType, Map<String, String> streamConfigMa
     }
     _flushAutotuneInitialRows = autotuneInitialRows > 0 ? autotuneInitialRows : DEFAULT_FLUSH_AUTOTUNE_INITIAL_ROWS;
 
-    String groupIdKey = StreamConfigProperties.constructStreamProperty(_type, StreamConfigProperties.GROUP_ID);
-    _groupId = streamConfigMap.get(groupIdKey);
+    String partitionRate = streamConfigMap.get(StreamConfigProperties.PARTITION_CONSUMPTION_RATE_LIMIT);
+    _partitionConsumptionRateLimit =
+        partitionRate != null ? Double.parseDouble(partitionRate) : CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED;

Review Comment:
   The new partition/topic rate limit parsing uses Double.parseDouble() without adding config-key context and will also accept non-finite values like NaN/Infinity. A malformed value would fail with an unhelpful NumberFormatException (or worse, silently disable throttling for NaN partition limits). Consider validating the value and throwing an IllegalArgumentException that includes the config key and value.



##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfigProperties.java:
##########
@@ -45,8 +45,7 @@ private StreamConfigProperties() {
   public static final String STREAM_IDLE_TIMEOUT_MILLIS = "idle.timeout.millis";
   public static final String STREAM_DECODER_CLASS = "decoder.class.name";
   public static final String DECODER_PROPS_PREFIX = "decoder.prop";
-  public static final String GROUP_ID = "hlc.group.id";
-  public static final String PARTITION_MSG_OFFSET_FACTORY_CLASS = "partition.offset.factory.class.name";
+  public static final String PARTITION_CONSUMPTION_RATE_LIMIT = "partition.consumption.rate.limit";
   public static final String TOPIC_CONSUMPTION_RATE_LIMIT = "topic.consumption.rate.limit";

Review Comment:
   Removing public constants from pinot-spi is a binary-incompatible change for third-party plugins that might reference these keys directly. Consider keeping the legacy keys as `@Deprecated` constants (even if Pinot no longer uses them) to preserve compatibility while still steering users to newer configs.



##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/RealtimeConsumptionRateManagerTest.java:
##########
@@ -91,22 +96,30 @@ public class RealtimeConsumptionRateManagerTest {
   @Test
   public void testCreateRateLimiter() {
     // topic A
-    ConsumptionRateLimiter rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_A, TABLE_NAME);
-    assertEquals(5.0, ((PartitionRateLimiter) rateLimiter).getRate(), DELTA);
+    ConsumptionRateLimiter rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_A, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 5.0, DELTA);
 
     // topic B
-    rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_B, TABLE_NAME);
-    assertEquals(2.5, ((PartitionRateLimiter) rateLimiter).getRate(), DELTA);
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_B, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 2.5, DELTA);
 
     // topic C
-    rateLimiter = _consumptionRateManager.createRateLimiter(STREAM_CONFIG_C, TABLE_NAME);
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_C, TABLE_NAME);
     assertEquals(rateLimiter, NOOP_RATE_LIMITER);
+
+    // topic D: partition level rate limit is used directly, without fetching the partition count
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_D, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 4.0, DELTA);
+
+    // topic E: partition level rate limit takes precedence over topic level rate limit
+    rateLimiter = CONSUMPTION_RATE_MANAGER.createRateLimiter(STREAM_CONFIG_E, TABLE_NAME);
+    assertEquals(((PartitionRateLimiter) rateLimiter).getRate(), 4.0, DELTA);

Review Comment:
   The new partition-level rate limit tests describe that the cache/partition-count fetch is skipped, but they don't currently assert that behavior. Adding a verify(times(0)) on the cache helps prevent regressions where partition-level throttling accidentally triggers a partition-count lookup.



##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -413,13 +413,14 @@ public int getFlushAutotuneInitialRows() {
     return _flushAutotuneInitialRows;
   }
 
-  public String getGroupId() {
-    return _groupId;
+  /// Returns the partition level consumption rate limit. Non-positive value means consumption is not throttled.
+  public double getPartitionConsumptionRateLimit() {
+    return _partitionConsumptionRateLimit;
   }
 
-  public Optional<Double> getTopicConsumptionRateLimit() {
-    return _topicConsumptionRateLimit == CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED ? Optional.empty()
-        : Optional.of(_topicConsumptionRateLimit);
+  /// Returns the topic level consumption rate limit. Non-positive value means consumption is not throttled.
+  public double getTopicConsumptionRateLimit() {
+    return _topicConsumptionRateLimit;

Review Comment:
   Changing getTopicConsumptionRateLimit() from Optional<Double> to double (and removing getGroupId()) in pinot-spi is a binary-incompatible API change for any external plugins compiled against the old signature. If the intent is to avoid a breaking change, consider keeping the old methods (deprecated) and introducing new methods with different names (e.g. getTopicConsumptionRateLimitValue()/getPartitionConsumptionRateLimitValue()), then migrate internal call sites first and remove the deprecated methods in a later major release.



-- 
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang merged PR #18904:
URL: https://github.com/apache/pinot/pull/18904


-- 
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.

xiangfu0 commented on PR #18904:
URL: https://github.com/apache/pinot/pull/18904#issuecomment-4895037198

   Opened the docs follow-up PR in pinot-docs: https://github.com/pinot-contrib/pinot-docs/pull/896


-- 
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang commented on code in PR #18904:
URL: https://github.com/apache/pinot/pull/18904#discussion_r3509494960


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -413,13 +413,14 @@ public int getFlushAutotuneInitialRows() {
     return _flushAutotuneInitialRows;
   }
 
-  public String getGroupId() {
-    return _groupId;
+  /// Returns the partition level consumption rate limit. Non-positive value means consumption is not throttled.
+  public double getPartitionConsumptionRateLimit() {
+    return _partitionConsumptionRateLimit;
   }
 
-  public Optional<Double> getTopicConsumptionRateLimit() {
-    return _topicConsumptionRateLimit == CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED ? Optional.empty()
-        : Optional.of(_topicConsumptionRateLimit);
+  /// Returns the topic level consumption rate limit. Non-positive value means consumption is not throttled.
+  public double getTopicConsumptionRateLimit() {
+    return _topicConsumptionRateLimit;

Review Comment:
   This is an intentional API change, noted in the PR description. There are no callers of these methods anywhere in the Pinot codebase, and `Optional` return is inconsistent with how other configs in this class are exposed. Keeping the deprecated `Optional` getter and adding parallel `*Value()` methods would leave permanent naming debt for a hypothetical external caller — not a good trade-off.



-- 
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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang commented on code in PR #18904:
URL: https://github.com/apache/pinot/pull/18904#discussion_r3509493794


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -198,11 +196,13 @@ public StreamConfig(String tableNameWithType, Map<String, String> streamConfigMa
     }
     _flushAutotuneInitialRows = autotuneInitialRows > 0 ? autotuneInitialRows : DEFAULT_FLUSH_AUTOTUNE_INITIAL_ROWS;
 
-    String groupIdKey = StreamConfigProperties.constructStreamProperty(_type, StreamConfigProperties.GROUP_ID);
-    _groupId = streamConfigMap.get(groupIdKey);
+    String partitionRate = streamConfigMap.get(StreamConfigProperties.PARTITION_CONSUMPTION_RATE_LIMIT);
+    _partitionConsumptionRateLimit =
+        partitionRate != null ? Double.parseDouble(partitionRate) : CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED;

Review Comment:
   This matches the existing parsing behavior of the topic level rate limit (and the general parsing style in this class). A malformed value throws `NumberFormatException` with the offending string at table config processing time, same as before. Non-finite values fall into the non-positive check and are treated as not throttled, which is a safe fallback. Not worth adding extra validation just for this 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@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] Add config to set consumption rate limit at partition level [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang commented on code in PR #18904:
URL: https://github.com/apache/pinot/pull/18904#discussion_r3509492761


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfigProperties.java:
##########
@@ -45,8 +45,7 @@ private StreamConfigProperties() {
   public static final String STREAM_IDLE_TIMEOUT_MILLIS = "idle.timeout.millis";
   public static final String STREAM_DECODER_CLASS = "decoder.class.name";
   public static final String DECODER_PROPS_PREFIX = "decoder.prop";
-  public static final String GROUP_ID = "hlc.group.id";
-  public static final String PARTITION_MSG_OFFSET_FACTORY_CLASS = "partition.offset.factory.class.name";
+  public static final String PARTITION_CONSUMPTION_RATE_LIMIT = "partition.consumption.rate.limit";
   public static final String TOPIC_CONSUMPTION_RATE_LIMIT = "topic.consumption.rate.limit";

Review Comment:
   These keys are dead: `hlc.group.id` is legacy from the long-removed high-level consumer, and `partition.offset.factory.class.name` is read by nothing in the codebase. Keeping a `@Deprecated` constant for a config key that no code consumes would be misleading — any plugin referencing them was already getting no behavior. The removal is called out in the PR description.



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