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


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -67,87 +74,155 @@ public static SendStatsPredicate create(PinotConfiguration 
configuration) {
       throw new IllegalArgumentException("Invalid value " + modeStr + " for "
           + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE, e);
     }
-    return mode.create();
+    return mode.create(helixManager);
   }
 
   public enum Mode {
     SAFE {
       @Override
-      public SendStatsPredicate create() {
-        return new Safe();
+      public SendStatsPredicate create(HelixManager helixManager) {
+        return new Safe(helixManager);
       }
     },
     ALWAYS {
       @Override
-      public SendStatsPredicate create() {
+      public SendStatsPredicate create(HelixManager helixManager) {
         return new SendStatsPredicate() {
           @Override
-          public boolean getSendStats() {
+          public boolean isSendStats() {
             return true;
           }
 
+          @Override
+          public boolean needWatchForInstanceConfigChange() {
+            return false;
+          }
+
           @Override
           public void onInstanceConfigChange(List<InstanceConfig> 
instanceConfigs, NotificationContext context) {
-            // Nothing to do
+            throw new UnsupportedOperationException("Should not be invoked");
           }
         };
       }
     },
     NEVER {
       @Override
-      public SendStatsPredicate create() {
+      public SendStatsPredicate create(HelixManager helixManager) {
         return new SendStatsPredicate() {
           @Override
-          public boolean getSendStats() {
+          public boolean isSendStats() {
+            return false;
+          }
+
+          @Override
+          public boolean needWatchForInstanceConfigChange() {
             return false;
           }
 
           @Override
           public void onInstanceConfigChange(List<InstanceConfig> 
instanceConfigs, NotificationContext context) {

Review Comment:
   No. I changed it to throw exception to ensure it is never invoked. It is 
registered only if `needWatchForInstanceConfigChange()` returns `true`



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

Reply via email to