[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener

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

jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r455993140



##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -413,6 +413,31 @@ public void addRoutingTableChangeListener(
     _routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
     logger.info("Attach RoutingTableProviderChangeListener {}",
         routingTableChangeListener.getClass().getName());
+    if (_sourceDataTypeMap.isEmpty()) {

Review comment:
       FYI, in Helix Manager, adding the listener will cause the listener to get a "free" callback when the callback handler is init. So the requirement here is doing the same for router.
   
   Given that saying, if the routing table has not been updated (maybe during the reading) then we return an empty snapshot. In this case, if the router still notifies the listener once the update is done, then we are still good. However, based on the code here, I think we might have a race condition that this notification could be sent later than the proper one. Then user would get the wrong information.
   So we need a concurrency control here.
   

##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -413,6 +413,31 @@ public void addRoutingTableChangeListener(
     _routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
     logger.info("Attach RoutingTableProviderChangeListener {}",
         routingTableChangeListener.getClass().getName());
+    if (_sourceDataTypeMap.isEmpty()) {
+      routingTableChangeListener.onRoutingTableChange(getRoutingTableSnapshot(), context);
+    } else {
+      for (PropertyType propertyType : _sourceDataTypeMap.keySet()) {

Review comment:
       As Junkai mentioned, I think we need a new interface for the listeners anyway.
   The current one won't tell the provider which type the listener wants to listen to. So I think we just add a new API so as to add the listeners with the right type.
   In this case, the old API needs to be deprecated and if user add their listen using this API, we shall just assign it to all the valid types.




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