somandal commented on code in PR #15392:
URL: https://github.com/apache/pinot/pull/15392#discussion_r2021451659
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/BaseSegmentOperationsThrottler.java:
##########
@@ -41,17 +46,22 @@ public abstract class BaseSegmentOperationsThrottler
implements PinotClusterConf
protected int _maxConcurrency;
protected int _maxConcurrencyBeforeServingQueries;
protected boolean _isServingQueries;
+ protected ServerGauge _thresholdGauge;
+ protected ServerGauge _countGauge;
+ private AtomicInteger _numSegmentsAcquiredSemaphore;
private final Logger _logger;
/**
* Base segment operations throttler constructor
* @param maxConcurrency configured concurrency
* @param maxConcurrencyBeforeServingQueries configured concurrency before
serving queries
* @param isServingQueries whether the server is ready to serve queries or
not
+ * @param thresholdGauge gauge metric to track the throttle thresholds
+ * @param countGauge gauge metric to track the number of segments undergoing
the given operation
* @param logger logger to use
*/
public BaseSegmentOperationsThrottler(int maxConcurrency, int
maxConcurrencyBeforeServingQueries,
- boolean isServingQueries, Logger logger) {
+ boolean isServingQueries, ServerGauge thresholdGauge, ServerGauge
countGauge, Logger logger) {
Review Comment:
All the segment operation throttlers extend from
`BaseSegmentOperationsThrottler`, and this class contains the main logic for
updating the configs and acquire / release of the semaphore. I pass in the
gauges here because each throttler has it's own gauge for threshold and count.
I thought this would be cleaner than adding if-else conditions.
Do you have any other recommendations other than passing them in? Other
option I could think of is adding methods for updating the gauge and have the
extenders implement them to set the value, while having the
`BaseSegmentOperationsThrottler` call into these methods. something like:
```
void updateThresholdGauge(int valueToSet);
void updateCountGauge(int valueToSet);
```
The actual implementation would look like:
```
void updateCountGauge(int valueToSet) {
_serverMetrics.setValueOfTableGauge(_tableNameWithType,
ServerGauge.SEGMENT_DOWNLOAD_COUNT, valueToSet)
}
```
wdyt?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
