vrajat commented on code in PR #15839:
URL: https://github.com/apache/pinot/pull/15839#discussion_r2097120853
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BrokerResourceOnlineOfflineStateModelFactory.java:
##########
@@ -116,14 +117,14 @@ public void onBecomeDroppedFromOffline(Message message,
NotificationContext cont
@Transition(from = "ONLINE", to = "DROPPED")
public void onBecomeDroppedFromOnline(Message message, NotificationContext
context) {
- String tableNameWithType = message.getPartitionName();
- LOGGER.info("Processing transition from ONLINE to DROPPED for table:
{}", tableNameWithType);
+ String physicalOrLogicalTable = message.getPartitionName();
+ LOGGER.info("Processing transition from ONLINE to DROPPED for table:
{}", physicalOrLogicalTable);
try {
- _routingManager.removeRouting(tableNameWithType);
- _queryQuotaManager.dropTableQueryQuota(tableNameWithType);
+ _routingManager.removeRouting(physicalOrLogicalTable);
Review Comment:
Same q. as the other comment in this file.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/QueryQuotaClusterIntegrationTest.java:
##########
@@ -333,11 +389,15 @@ private void runQueries(int qps, boolean shouldFail,
String applicationName) {
private static volatile String _quotaSource;
private void verifyQuotaUpdate(double quotaQps) {
+ verifyQuotaUpdateWithTableName(quotaQps, getTableName() + "_OFFLINE");
+ }
+
+ private void verifyQuotaUpdateWithTableName(double quotaQps, String
tableName) {
try {
TestUtils.waitForCondition(aVoid -> {
try {
double tableQuota = Double.parseDouble(sendGetRequest(
- "http://"; + _brokerHostPort + "/debug/tables/queryQuota/" +
getTableName() + "_OFFLINE"));
+ "http://"; + _brokerHostPort + "/debug/tables/queryQuota/" +
tableName));
Review Comment:
Logical table query quota is also checked using the same API ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BrokerResourceOnlineOfflineStateModelFactory.java:
##########
@@ -97,14 +98,14 @@ public void onBecomeOnlineFromOffline(Message message,
NotificationContext conte
@Transition(from = "ONLINE", to = "OFFLINE")
public void onBecomeOfflineFromOnline(Message message, NotificationContext
context) {
- String tableNameWithType = message.getPartitionName();
- LOGGER.info("Processing transition from ONLINE to OFFLINE for table:
{}", tableNameWithType);
+ String physicalOrLogicalTable = message.getPartitionName();
+ LOGGER.info("Processing transition from ONLINE to OFFLINE for table:
{}", physicalOrLogicalTable);
try {
- _routingManager.removeRouting(tableNameWithType);
- _queryQuotaManager.dropTableQueryQuota(tableNameWithType);
+ _routingManager.removeRouting(physicalOrLogicalTable);
Review Comment:
Will this cause a log to be printed for logical table that there is no
routing for it ? Can you add an if condition so that the log files are not
misleading ? Was this function always being called and we missed handling
logical tables correctly here ?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
