[GitHub] [helix] pkuwm commented on a change in pull request #1550: Controller-side Task Current State Migration

[GitBox <gi...@apache.org>]

pkuwm commented on a change in pull request #1550:
URL: https://github.com/apache/helix/pull/1550#discussion_r533006447



##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -229,6 +229,15 @@ void addCurrentStateChangeListener(CurrentStateChangeListener listener, String i
   void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeListener listener, String instanceName,
       String sessionId) throws Exception;
 
+  /**
+   * Uses CurrentStateChangeListener since TaskCurrentState shares the same CurrentState model
+   * @see CurrentStateChangeListener#onStateChange(String, List, NotificationContext)
+   * @param listener
+   * @param instanceName
+   */
+  void addTaskCurrentStateChangeListener(CurrentStateChangeListener listener, String instanceName,

Review comment:
       I would suggest we add a default implementation for this newly added API for backwards compatibility. I've noticed that a helix user **ambry** implements this interface. So if we don't add a default implementation, it'll break ambry build if they upgrades helix.
   And another benefit of default implementation is, we don't need to add an empty implementation in those classes like `MockClusterManager` in tests. Thus less changed files. 




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org