yashmayya commented on code in PR #18288:
URL: https://github.com/apache/pinot/pull/18288#discussion_r3197671522
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManagerTest.java:
##########
@@ -388,6 +388,45 @@ public void testStatsMetricExportDisabled() throws
InterruptedException {
assertNull(_brokerMetrics.getGaugeValue(numInFlightKey));
}
+ @Test
+ public void testHybridScoreWithQueueSizeOffset() {
Review Comment:
Should this compute multiple server scores to cover the scenario mentioned
in the PR (multiple idle servers should still be ranked by latency)?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1126,6 +1126,9 @@ public enum Type {
// Parameters related to Hybrid score.
public static final String CONFIG_OF_HYBRID_SCORE_EXPONENT =
CONFIG_PREFIX + ".hybrid.score.exponent";
public static final int DEFAULT_HYBRID_SCORE_EXPONENT = 3;
+ public static final String CONFIG_OF_HYBRID_SCORE_QUEUE_SIZE_OFFSET =
+ CONFIG_PREFIX + ".hybrid.score.queue.size.offset";
+ public static final int DEFAULT_HYBRID_SCORE_QUEUE_SIZE_OFFSET = 0;
Review Comment:
This config name isn't very intuitive IMO, are there any simplified
alternatives?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1126,6 +1126,9 @@ public enum Type {
// Parameters related to Hybrid score.
public static final String CONFIG_OF_HYBRID_SCORE_EXPONENT =
CONFIG_PREFIX + ".hybrid.score.exponent";
public static final int DEFAULT_HYBRID_SCORE_EXPONENT = 3;
+ public static final String CONFIG_OF_HYBRID_SCORE_QUEUE_SIZE_OFFSET =
Review Comment:
nit: might be worth adding a comment here explaining this config.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsEntry.java:
##########
@@ -58,6 +61,7 @@ public ServerRoutingStatsEntry(String serverInstanceId,
double alphaEMA, long au
periodicTaskExecutor);
_hybridScoreExponent = scoreExponent;
+ _hybridScoreQueueSizeOffset = queueSizeOffset;
Review Comment:
Let's validate that this is non-negative?
--
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]
