You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/07/23 10:36:09 UTC
[GitHub] [helix] pkuwm commented on a change in pull request #1154: Add initial callback when adding routing table listener
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org