Jackie-Jiang commented on code in PR #8572:
URL: https://github.com/apache/pinot/pull/8572#discussion_r854759946
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/ClusterChangeMediator.java:
##########
@@ -210,21 +206,21 @@ public void onLiveInstanceChange(List<LiveInstance>
liveInstances, NotificationC
*
* @param changeType Type of the change
*/
- private synchronized void enqueueChange(ChangeType changeType) {
+ private void enqueueChange(ChangeType changeType) {
// Do not enqueue or process changes if already stopped
- if (_stopped) {
+ if (!_running) {
+ LOGGER.warn("ClusterChangeMediator already stopped, skipping enqueuing
the {} change", changeType);
return;
}
if (_clusterChangeHandlingThread.isAlive()) {
- LOGGER.info("Enqueue {} change", changeType);
- synchronized (_lastChangeTimeMap) {
- if (!_lastChangeTimeMap.containsKey(changeType)) {
- _lastChangeTimeMap.put(changeType, System.currentTimeMillis());
+ LOGGER.info("Enqueueing {} change", changeType);
+ if (_lastChangeTimeMap.putIfAbsent(changeType,
System.currentTimeMillis()) == null) {
+ synchronized (_lastChangeTimeMap) {
_lastChangeTimeMap.notify();
Review Comment:
I understand that using `notifyAll()` can prevent certain deadlock
situations, but in the current code there is only one single thread that can
wait on this lock. Do you see a problem using `notify()` in this case?
--
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
