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/15 17:00:44 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #1154: Add initial callback when adding routing table listener
zhangmeng916 opened a new pull request #1154:
URL: https://github.com/apache/helix/pull/1154
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
(#1090 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR adds an initial callback when user adds a listener to Helix routing table. This initial callback will ensure uses are able to perform operations, e.g. read routing table data or some other actions, immediately after they add the listener instead of waiting for the first data change to trigger the callback.
### Tests
- [X] The following tests are modified for this issue:
TestRoutingTableProvider.java
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [X] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [X] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r462070242
##########
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:
You could read this article: https://stackoverflow.com/questions/24367472/is-an-object-created-first-and-then-its-constructor-executed. Pay attention to point 4 - although taking a closer look, I don't think this leaks itself as `this`. We actually see this in the commit: https://github.com/apache/helix/commit/ff0485bdb65d1051ca63ecc97c5b864aee932465
##########
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(
Review comment:
I'm not quite seeing that - applications should program in a way that expects some data in the snapshot, right? Their logic shouldn't try to "time it" in their callback logic since that's never predictable.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by 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
[GitHub] [helix] dasahcc commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456062288
##########
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:
> So the users that would like the initial callback will explicitly call the new API?
Yes.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r457134295
##########
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(
Review comment:
This is a high-level question. Have we considered doing init as the default behavior? Here are the reasons I can think off the top of my head:
1. It doesn't hurt to init (what are the bad side effects of reading in the beginning? If the user wishes to add a listener, that means the user is ready to read and be notified). In other words, I can't think of any negative consequences of this behavior change or any potential backward-compatibility issues that might break things.
2. This greatly reduces verbosity in the code. Simple is better.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1154:
URL: https://github.com/apache/helix/pull/1154#issuecomment-667829706
@zhangmeng916
Please follow the pull request description guidelines. It's recommended that we use the following keywords:
close
closes
closed
fix
fixes
fixed
resolve
resolves
resolved
when referring to the issue number. This is clearly outlined in our guidelines so that when the PR gets closed, the issue gets linked and closed as well. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r494575642
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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
Review comment:
nit, will "doTriggerCallback" easier to read from the users' perspective?
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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());
+ logger.info(
+ "Attach RoutingTableProviderChangeListener {}. The doInit value of this listener is {}",
Review comment:
nit, we can split "The doInit value of this listener is {}" part. And move it into the "if" code block below. Might be cleaner.
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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 isTriggerCallback whether to trigger the initial callback during adding listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object context,
+ boolean isTriggerCallback) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
- logger.info("Attach RoutingTableProviderChangeListener {}",
- routingTableChangeListener.getClass().getName());
+ logger.info("Attach RoutingTableProviderChangeListener {}.",
+ routingTableChangeListener.getClass().getName(), isTriggerCallback);
+ if (isTriggerCallback) {
+ logger.info("The isTriggerCallback value for the listener is true");
Review comment:
What I meant is that we can log something like "Force triggering a callback for the new listener" here. So the log does not care about the name of the parameter "isTriggerCallback".
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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 isTriggerCallback whether to trigger the initial callback during adding listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object context,
+ boolean isTriggerCallback) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
- logger.info("Attach RoutingTableProviderChangeListener {}",
- routingTableChangeListener.getClass().getName());
+ logger.info("Attach RoutingTableProviderChangeListener {}.",
+ routingTableChangeListener.getClass().getName(), isTriggerCallback);
Review comment:
nit, remove the isTriggerCallback that is not used in the log.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r464217408
##########
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:
Sorry if my comment wasn't clear. What I was saying was since `RoutingTableProvider` doesn't give its own reference and doesn't implement `RoutingTableChangeListener`, we probably don't have to worry about this and we shouldn't have to add a null check here. I think it should be okay as is :)
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456041505
##########
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:
For example, this constructor allows the user to create a map of types.
```
public RoutingTableProvider(HelixManager helixManager, Map<PropertyType, List<String>> sourceDataTypeMap) {
this(helixManager, sourceDataTypeMap, true, DEFAULT_PERIODIC_REFRESH_INTERVAL);
}
```
But we don't have a way for the user to specify the listener for different types. So one listener will be triggered multiple times if multiple types are specified.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456627107
##########
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:
I agree this is out of this PR's scope. My main concern is that this PR adds one more place that we need to modify later. It would be great if we have only one method to trigger all onRoutingTableChange calls.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456942125
##########
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:
Yeah, the onRoutingTableChange is called in both add listener and whenever there is any change triggers it. These two won't be able to be merged, right?
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on pull request #1154:
URL: https://github.com/apache/helix/pull/1154#issuecomment-698650095
This PR is ready to merge. Approved by @jiajunwang
Final commit message:
Add initial callback when adding routing table listener.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r455981735
##########
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:
Let's not change the API. The other customers can still use this API. But let's have another API and old API call the new API with input value as false (for first time refresh) to guarantee the backward compatible.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang merged pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1154:
URL: https://github.com/apache/helix/pull/1154
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456018394
##########
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:
Why would we need the type as input? We already put the type in _sourceDataTypeMap when users initialize the routing table provider, right? So we can trigger callback with required type of snapshot.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r494674523
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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 isTriggerCallback whether to trigger the initial callback during adding listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object context,
+ boolean isTriggerCallback) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
- logger.info("Attach RoutingTableProviderChangeListener {}",
- routingTableChangeListener.getClass().getName());
+ logger.info("Attach RoutingTableProviderChangeListener {}.",
+ routingTableChangeListener.getClass().getName(), isTriggerCallback);
+ if (isTriggerCallback) {
+ logger.info("The isTriggerCallback value for the listener is true");
Review comment:
What I meant is that we can log something like "Force triggering a callback for the new listener" here. So the log does not care about the name of the parameter "isTriggerCallback".
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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 isTriggerCallback whether to trigger the initial callback during adding listener
+ */
+ public void addRoutingTableChangeListener(
+ final RoutingTableChangeListener routingTableChangeListener, Object context,
+ boolean isTriggerCallback) {
_routingTableChangeListenerMap.put(routingTableChangeListener, new ListenerContext(context));
- logger.info("Attach RoutingTableProviderChangeListener {}",
- routingTableChangeListener.getClass().getName());
+ logger.info("Attach RoutingTableProviderChangeListener {}.",
+ routingTableChangeListener.getClass().getName(), isTriggerCallback);
Review comment:
nit, remove the isTriggerCallback that is not used in the log.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r494575642
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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
Review comment:
nit, will "doTriggerCallback" easier to read from the users' perspective?
##########
File path: helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
##########
@@ -403,16 +407,35 @@ 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());
+ logger.info(
+ "Attach RoutingTableProviderChangeListener {}. The doInit value of this listener is {}",
Review comment:
nit, we can split "The doInit value of this listener is {}" part. And move it into the "if" code block below. Might be cleaner.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r455967372
##########
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:
I see your concern. There're several customers that wanted this feature. However, it's possible to cause unnecessary callback to other customers. Maybe we have a boolean value in the function, and let customers to determine their usage?
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r464013101
##########
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:
@zhangmeng916 Then are we checking null for sourceMap in validateSourceDataTypeMap?
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r457520882
##########
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(
Review comment:
@dasahcc had some concerns that if users are not expecting an empty snapshot, and the initial callback provides them such, they may not have a way to handle it yet.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r464523917
##########
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(
Review comment:
let's quickly sync offline.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on pull request #1154:
URL: https://github.com/apache/helix/pull/1154#issuecomment-698650095
This PR is ready to merge. Approved by @jiajunwang
Final commit message:
Add initial callback when adding routing table listener.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r457131946
##########
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:
If `_sourceDataTypeMap ` is not properly initialized in the constructor (if this method gets called before the object is fully initialized), this might throw an NPE. I think we should add a null check here on the map. This will guard against potential concurrency issues (that would be pretty hard to debug but might happen regardless).
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r463306494
##########
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:
Thanks for the information. Didn't know this before.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r460648477
##########
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:
@pkuwm I think it makes sense. @narendly could you please let us know when it is possible that adding the listener happens before routing table provider is fully initialized.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r456610082
##########
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:
I see your point, so right now although the routing table provider only initialized with certain source data types, users will still get callback on all snapshot change, and they need to filter based on the type to see whether the change is they want. But I think this change is beyond the scope of this PR. It's always a problem after we have multiple types, all the Zookeeper change will trigger callback, if we want to change, we will need to distinguish between different listeners, and the change also needs to be made in `notifyRoutingTableChange` when we receive a change.
For this PR only, we do trigger callback based on types, and user won't get unused callback for those types they don't care. For more general type support for callback, we'll need more changes. Thoughts?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r464217408
##########
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:
Sorry if my comment wasn't clear. What I was saying was since `RoutingTableProvider` doesn't give away its own reference and doesn't implement `RoutingTableChangeListener`, we probably don't have to worry about this and we shouldn't have to add a null check here. I think it should be okay as is :)
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r455963147
##########
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:
This is an initial call when adding listener. The only concern is returning empty data should be a good signal for user? This changes behavior. Some users are relying on the snapshot content. Shall we have a different API that give the option to let user choose it?
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r455983399
##########
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:
So the users that would like the initial callback will explicitly call the new API?
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1154: Add initial callback when adding routing table listener
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1154:
URL: https://github.com/apache/helix/pull/1154#discussion_r457521558
##########
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:
I added the null check. Just to make sure, you're concerning the race condition between add routing table listener and the routing table provider construction?
----------------------------------------------------------------
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