pkuwm commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r459322041
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/spectator/TestRoutingTableProvider.java
##########
@@ -178,7 +182,19 @@ public void afterClass() {
deleteCluster(CLUSTER_NAME);
}
- @Test
+ @Test()
+ public void testInvocation() throws Exception {
+ MockRoutingTableChangeListener routingTableChangeListener =
+ new TestRoutingTableProvider().new MockRoutingTableChangeListener();
Review comment:
`new TestRoutingTableProvider()` is unnecessary?
##########
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +403,55 @@ public RoutingTableSnapshot
getRoutingTableSnapshot(PropertyType propertyType, S
return snapshots;
}
+
/**
* Add RoutingTableChangeListener with user defined context
* @param routingTableChangeListener
* @param context user defined context
*/
public void addRoutingTableChangeListener(
final RoutingTableChangeListener routingTableChangeListener, Object
context) {
+ addRoutingTableChangeListener(routingTableChangeListener, context, false);
+ }
+
+ /**
+ * Add RoutingTableChangeListener with user defined context
+ * @param routingTableChangeListener
+ * @param context user defined context
+ * @param doInit whether to trigger the initial callback during adding
listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object
context, boolean doInit) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new
ListenerContext(context));
logger.info("Attach RoutingTableProviderChangeListener {}",
routingTableChangeListener.getClass().getName());
+ if (doInit && _sourceDataTypeMap != null) {
Review comment:
Maybe also add `doInit` value in the log?
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/spectator/TestRoutingTableProvider.java
##########
@@ -178,7 +182,18 @@ public void afterClass() {
deleteCluster(CLUSTER_NAME);
}
- @Test
+ @Test()
Review comment:
Nit, no need to have `()`
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/spectator/TestRoutingTableProvider.java
##########
@@ -178,7 +182,18 @@ public void afterClass() {
deleteCluster(CLUSTER_NAME);
}
- @Test
+ @Test()
+ public void testInvocation() throws Exception {
+ MockRoutingTableChangeListener routingTableChangeListener =
+ new TestRoutingTableProvider().new MockRoutingTableChangeListener();
+
_routingTableProvider_default.addRoutingTableChangeListener(routingTableChangeListener,
null);
+
+ // Initial add listener should trigger the first execution of the
+ // listener callbacks
+
AssertJUnit.assertTrue(routingTableChangeListener.routingTableChangeReceived);
Review comment:
`Assert.assertTrue`? So no need to import `AssertJUnit`.
##########
File path:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +403,55 @@ public RoutingTableSnapshot
getRoutingTableSnapshot(PropertyType propertyType, S
return snapshots;
}
+
/**
* Add RoutingTableChangeListener with user defined context
* @param routingTableChangeListener
* @param context user defined context
*/
public void addRoutingTableChangeListener(
final RoutingTableChangeListener routingTableChangeListener, Object
context) {
+ addRoutingTableChangeListener(routingTableChangeListener, context, false);
+ }
+
+ /**
+ * Add RoutingTableChangeListener with user defined context
+ * @param routingTableChangeListener
+ * @param context user defined context
+ * @param doInit whether to trigger the initial callback during adding
listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object
context, boolean doInit) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new
ListenerContext(context));
logger.info("Attach RoutingTableProviderChangeListener {}",
routingTableChangeListener.getClass().getName());
+ if (doInit) {
+ if (_sourceDataTypeMap.isEmpty()) {
Review comment:
Just two cents: maybe I am wrong but I don't think it could happen: `if
this method gets called before the object is fully initialized`. The object
should already be fully initialized when we are able to call this method:
`table.addRoutingTableChangeListener()`.
It is a still good point to check null. By checking the code, I think the
only situation when `_sourceDataTypeMap` is null is, constructor `public
RoutingTableProvider(HelixManager helixManager,
Map<PropertyType, List<String>> sourceDataTypeMap, boolean
isPeriodicRefreshEnabled,
long periodRefreshInterval)` is directly called and sourceDataTypeMap
is passed as null.
In this case, we should validate the arguments in the constructor instead of
in this method: there are many other places that call _sourceDataMap. It is not
a good idea to do every null check for class variables in each method. Since we
already have `validateSourceDataTypeMap()` but it doesn't check null for
`validateSourceDataTypeMap`, I think we should add the check to
`validateSourceDataTypeMap()` instead of here.
```
if (sourceDataTypeMap == null) {
throw new IllegalArgumentException();
}
```
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
