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 2025/05/24 00:03:38 UTC
[PR] Reduce ZK access for SendStatsPredicate [pinot]
Jackie-Jiang opened a new pull request, #15895:
URL: https://github.com/apache/pinot/pull/15895
Fix #15890
- For `ALWAYS` and `NEVER`, no need to register it as listener
- For `SAFE`, disable pre-fetch and only read the changed instance config
No extra test is added, but the existing integration tests should cover the changed code
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2914031858
I suggest looking at mechanisms other than watches. It may be slower to react, but at least it wont cause production outages due to zookeeper falling over.
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2109774386
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
Checking the Helix code, and `null` is not really possible:
```
@Override
public synchronized HelixAdmin getClusterManagmentTool() {
checkConnected(_waitForConnectedTimeout);
if (_zkclient != null && !_zkclient.isClosed()) {
return new ZKHelixAdmin(_zkclient);
}
LOG.error("Couldn't get ZKClusterManagementTool because zkclient is null");
return null;
}
```
```
void checkConnected(long timeout) {
if (_zkclient == null || _zkclient.isClosed()) {
throw new HelixException(
"HelixManager (ZkClient) is not connected. Call HelixManager#connect()");
}
boolean isConnected = isConnected();
if (!isConnected && timeout > 0) {
LOG.warn("zkClient to " + getZkConnectionInfo() + " is not connected, wait for "
+ _waitForConnectedTimeout + "ms.");
isConnected = _zkclient.waitUntilConnected(_waitForConnectedTimeout, TimeUnit.MILLISECONDS);
}
if (!isConnected) {
LOG.error("zkClient is not connected after waiting " + timeout + "ms."
+ ", clusterName: " + _clusterName + ", zkAddress: " + getZkConnectionInfo());
throw new HelixException(
"HelixManager is not connected within retry timeout for cluster " + _clusterName);
}
}
```
The implementation is confusing, but the contract should be either throwing exception or returning non-null result.
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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-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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "anandheritage (via GitHub)" <gi...@apache.org>.
anandheritage commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2105660969
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
Also looks getClusterManagmentTool - spelling is incorrect
It should be getClusterManagementTool
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
getClusterManagmentTool - Can return null - This needs to be handled
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2108348994
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
Even null can be returned... can be returned when it is called from here? If null is possible, we should deal with that possibility 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-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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2914467426
> @mcvsubbu To workaround the overhead of watches, you can manually set `pinot.query.mse.stats.mode` to `ALWAYS` or `NEVER`
I am guessing that both of these have a cost, and I am trying to understand what that cost is. Is it that if I set it to ALWAYS, then there will be some incompatibility when queries are processed in MSE? Or, if I set it to NEVER, MSE processing will be sub-optimal?
It is a big ask to set watches on _all_ nodes in the cluster, so I am trying to understand what that buys someone.
@gortiz ?
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2917663715
Then we should definitely find a better way of handling safe/unsafe versions. The users may not be able to complete their upgrade process (like it happened for us).
If this is a single unsafe/safe crossover, then let us introduce a config that users can roll once they are in safe version. We can even provide a controller API if needed to assure that they are indeed in safe version. The users can then do a one-time restart with the new config.
Other suggestions/thoughts welcome, but setting a watch on every node in the cluster is a tall ask.
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2917213249
It is purely for backward compatible handling, and there is no overhead for `ALWAYS` or `NEVER`.
I guess we can change the default to `ALWAYS` once the version is upgraded to a safe version, and user can use it if they need to upgrade from an unsafe version
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "anandheritage (via GitHub)" <gi...@apache.org>.
anandheritage commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2105660969
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
getClusterManagmentTool - spelling is incorrect
It should be getClusterManagementTool
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2107735658
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
From the Helix code, seems it can return `null`, but we always assume it can never return `null` across the Pinot code base (it is used across a lot of components). Would prefer keeping the same assumption 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-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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2914040443
@mcvsubbu To workaround the overhead of watches, you can manually set `pinot.query.mse.stats.mode` to `ALWAYS` or `NEVER`
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2107735913
##########
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) {
- // Nothing to do
+ throw new UnsupportedOperationException("Should not be invoked");
}
};
}
};
- public abstract SendStatsPredicate create();
+ public abstract SendStatsPredicate create(HelixManager helixManager);
}
+ @BatchMode(enabled = false)
+ @PreFetch(enabled = false)
private static class Safe extends SendStatsPredicate {
- private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+ private final HelixManager _helixManager;
+ private final String _clusterName;
+ private final Map<String, String> _problematicVersionsById = new HashMap<>();
+
+ private HelixAdmin _helixAdmin;
+ private volatile boolean _sendStats = true;
+
+ public Safe(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _clusterName = helixManager.getClusterName();
+ }
+
+ @Override
+ public boolean isSendStats() {
+ return _sendStats;
+ }
@Override
- public boolean getSendStats() {
- return _sendStats.get();
+ public boolean needWatchForInstanceConfigChange() {
+ return true;
}
@Override
- public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
- Map<String, String> problematicVersionsById = new HashMap<>();
- for (InstanceConfig instanceConfig : instanceConfigs) {
- switch (InstanceTypeUtils.getInstanceType(instanceConfig.getInstanceName())) {
- case BROKER:
- case SERVER:
- String otherVersion = instanceConfig.getRecord()
- .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
- if (isProblematicVersion(otherVersion)) {
- problematicVersionsById.put(instanceConfig.getInstanceName(), otherVersion);
+ public synchronized void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ if (_helixAdmin == null) {
+ _helixAdmin = _helixManager.getClusterManagmentTool();
Review Comment:
This API is from Helix, so I cannot really change it
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2906147678
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15895?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 56.20%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6e33638`)](https://app.codecov.io/gh/apache/pinot/commit/6e3363830f01e3a0d00a291a3850c518af4e5156?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 133 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (6e33638). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (6e33638) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15895 +/- ##
============================================
- Coverage 62.90% 56.20% -6.70%
+ Complexity 1386 690 -696
============================================
Files 2867 2270 -597
Lines 163354 123834 -39520
Branches 24952 19740 -5212
============================================
- Hits 102755 69605 -33150
+ Misses 52847 48479 -4368
+ Partials 7752 5750 -2002
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15895/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/15895/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/15895/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/15895/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/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15895/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/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.20% <ø> (-6.62%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.20% <ø> (-6.70%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.20% <ø> (-6.70%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.20% <ø> (+0.38%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15895/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/15895?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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2914011383
This mechanism is introduced to tolerate backward incompatible changes for query stats. @gortiz Do you foresee future changes that might be backward incompatible? Given this is adding overhead to ZK, we should revisit this mechanism.
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on code in PR #15895:
URL: https://github.com/apache/pinot/pull/15895#discussion_r2109764812
##########
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:
@Jackie-Jiang is it still setting watches?
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #15895:
URL: https://github.com/apache/pinot/pull/15895
--
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] Reduce ZK access for SendStatsPredicate [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15895:
URL: https://github.com/apache/pinot/pull/15895#issuecomment-2934712451
If this is a single unsafe/safe crossover, then let us introduce a config that users can roll once they are in safe version. We can even provide a controller API if needed to assure that they are indeed in safe version. The users can then do a one-time restart with the new config.
The idea is precisely to avoid that. We have tons of different configs for different features that were designed based on that pattern, where the operators need to be aware of the release notes, then they need to be sure the config has a specific value and finally set the config back to the final value after the release is complete. And sometimes the operators need to restart the cluster after that to apply the new config.
This process doesn't scale. Pinot is progressing much faster now, and asking humans to be aware of all these config changes is error-prone.
Instead, we can detect whether an instance is on a heterogeneous cluster or not. This should be a cheap operation. Perhaps the current implementation is not the most effective. I'm not an expert on Helix, and there may be cheaper ways to receive a notification when a new instance joins the cluster. But the theoretical concept of what we are trying to implement seems pretty simple and it must be a way to get notified without investing too many resources. I'm open to new ideas or implementations, but I don't think we should resign to the old manual and error-prone way to upgrade our system.
--
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