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/08/29 00:48:27 UTC
[GitHub] [helix] pkuwm opened a new pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
pkuwm opened a new pull request #1332:
URL: https://github.com/apache/helix/pull/1332
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1331
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
PreFetch annotation in class CallbackHandler is using `org.apache.helix.api.listeners.PreFetch`.
Then when it is disabled, the value is not honor in ZkClient. So ZkClient still prefetches the data for callback handler. Because in ZkClient, the annotation org.apache.helix.zookeeper.zkclient.annotation.PreFetch is used.
This PR also changes the existing annotation in zookeeper-api `org.apache.helix.zookeeper.zkclient.annotation.PreFetch` is proposed to `org.apache.helix.zookeeper.zkclient.annotation.PreFetchChangedData`.
### Tests
- [ ] The following tests are written for this issue:
- TestPrefetchChangedData
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Before CI test pass, please copy & paste the result of "mvn test")
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- 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
- 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] pkuwm commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480401890
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * If disabled({@code false}): ZkClient will not read data, so data object is passed as null.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ *
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change: data is null in
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}
+ */
@Retention(RetentionPolicy.RUNTIME)
Review comment:
`Prefetch` is Helix's logic, should not be involved here in zkClient. The javadoc should stay in that interface, not this one.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479587081
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -102,8 +102,7 @@
import static org.apache.helix.HelixConstants.ChangeType.RESOURCE_CONFIG;
import static org.apache.helix.HelixConstants.ChangeType.TARGET_EXTERNAL_VIEW;
-
-@PreFetch(enabled = false)
+@PreFetchChangedData(enabled = false)
Review comment:
Add a test to protect it. If it is not honored, the test fails.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480378870
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -102,8 +102,7 @@
import static org.apache.helix.HelixConstants.ChangeType.RESOURCE_CONFIG;
import static org.apache.helix.HelixConstants.ChangeType.TARGET_EXTERNAL_VIEW;
-
-@PreFetch(enabled = false)
+@PreFetchChangedData(enabled = false)
Review comment:
@kaisun2000 Double verify here: https://github.com/apache/helix/issues/1331#issuecomment-683212819
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480388496
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestPrefetchChangedData.java
##########
@@ -0,0 +1,109 @@
+package org.apache.helix.zookeeper.zkclient;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.impl.TestHelper;
+import org.apache.helix.zookeeper.impl.ZkTestBase;
+import org.apache.helix.zookeeper.impl.client.ZkClient;
+import org.apache.helix.zookeeper.zkclient.annotation.PreFetchChangedData;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class TestPrefetchChangedData extends ZkTestBase {
+ @Test
+ public void testPrefetchChangedDataEnabled() throws InterruptedException {
+ String path = "/" + TestHelper.getTestMethodName();
+ ZkClient zkClient = new ZkClient(ZkTestBase.ZK_ADDR);
+
+ try {
+ zkClient.createPersistent(path, "v1");
+
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ PreFetchZkDataListener dataListener = new PreFetchZkDataListener(countDownLatch);
+ zkClient.subscribeDataChanges(path, dataListener);
+ zkClient.writeData(path, "v2");
+
+ Assert.assertTrue(countDownLatch.await(3L, TimeUnit.SECONDS));
Review comment:
Cool. Due to recent fixing unstable test, this is getting to be a habit to look for unstableness.
Let us define a constant of this waiting for both test cases. Otherwise, looks good to me.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480384049
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * If disabled({@code false}): ZkClient will not read data, so data object is passed as null.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ *
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change: data is null in
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}
+ */
@Retention(RetentionPolicy.RUNTIME)
Review comment:
@kaisun2000 It tells the Java compiler and JVM that the annotation should be available via reflection at runtime/class/source code. More details you could check jdk javadoc.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479586914
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,19 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change.
+ */
Review comment:
Sure. I'll this.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479687119
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * By default, prefetch is disabled: ZkClient will not read data, so data object is passed as null.
Review comment:
I don't know how I wrote two conflicting sentences @____@ Addressed.
----------------------------------------------------------------
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 pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1332:
URL: https://github.com/apache/helix/pull/1332#issuecomment-684146206
This PR is ready to be merged, approved by @kaisun2000
----------------------------------------------------------------
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] lei-xia commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479673927
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * By default, prefetch is disabled: ZkClient will not read data, so data object is passed as null.
Review comment:
By default, prefetch is enabled or disabled? Line 28 and 31 are conflict.
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480384480
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestPrefetchChangedData.java
##########
@@ -0,0 +1,109 @@
+package org.apache.helix.zookeeper.zkclient;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.impl.TestHelper;
+import org.apache.helix.zookeeper.impl.ZkTestBase;
+import org.apache.helix.zookeeper.impl.client.ZkClient;
+import org.apache.helix.zookeeper.zkclient.annotation.PreFetchChangedData;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class TestPrefetchChangedData extends ZkTestBase {
+ @Test
+ public void testPrefetchChangedDataEnabled() throws InterruptedException {
+ String path = "/" + TestHelper.getTestMethodName();
+ ZkClient zkClient = new ZkClient(ZkTestBase.ZK_ADDR);
+
+ try {
+ zkClient.createPersistent(path, "v1");
+
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ PreFetchZkDataListener dataListener = new PreFetchZkDataListener(countDownLatch);
+ zkClient.subscribeDataChanges(path, dataListener);
+ zkClient.writeData(path, "v2");
+
+ Assert.assertTrue(countDownLatch.await(3L, TimeUnit.SECONDS));
Review comment:
@kaisun2000 await(3L, TimeUnit.**SECONDS**): it is 3 seconds.
----------------------------------------------------------------
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 merged pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1332:
URL: https://github.com/apache/helix/pull/1332
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480389158
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * If disabled({@code false}): ZkClient will not read data, so data object is passed as null.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ *
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change: data is null in
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}
+ */
@Retention(RetentionPolicy.RUNTIME)
Review comment:
By the way, just to confirm. After this fix, PreFetchChangeData is used to ZKClient? How about prefetch, who is using it? can you add comment here?
----------------------------------------------------------------
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 #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479587081
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -102,8 +102,7 @@
import static org.apache.helix.HelixConstants.ChangeType.RESOURCE_CONFIG;
import static org.apache.helix.HelixConstants.ChangeType.TARGET_EXTERNAL_VIEW;
-
-@PreFetch(enabled = false)
+@PreFetchChangedData(enabled = false)
Review comment:
Add a test to protect it. If it is not honored, the test fails.
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480387479
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * If disabled({@code false}): ZkClient will not read data, so data object is passed as null.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ *
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change: data is null in
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}
+ */
@Retention(RetentionPolicy.RUNTIME)
Review comment:
Right, is it a must here? I don't see we have something like this in other places, say participant side?
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480377256
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -102,8 +102,7 @@
import static org.apache.helix.HelixConstants.ChangeType.RESOURCE_CONFIG;
import static org.apache.helix.HelixConstants.ChangeType.TARGET_EXTERNAL_VIEW;
-
-@PreFetch(enabled = false)
+@PreFetchChangedData(enabled = false)
Review comment:
Previously (in production) callbackhandler not using prefetch, do we verify?
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480378705
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestPrefetchChangedData.java
##########
@@ -0,0 +1,109 @@
+package org.apache.helix.zookeeper.zkclient;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.impl.TestHelper;
+import org.apache.helix.zookeeper.impl.ZkTestBase;
+import org.apache.helix.zookeeper.impl.client.ZkClient;
+import org.apache.helix.zookeeper.zkclient.annotation.PreFetchChangedData;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class TestPrefetchChangedData extends ZkTestBase {
+ @Test
+ public void testPrefetchChangedDataEnabled() throws InterruptedException {
+ String path = "/" + TestHelper.getTestMethodName();
+ ZkClient zkClient = new ZkClient(ZkTestBase.ZK_ADDR);
+
+ try {
+ zkClient.createPersistent(path, "v1");
+
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ PreFetchZkDataListener dataListener = new PreFetchZkDataListener(countDownLatch);
+ zkClient.subscribeDataChanges(path, dataListener);
+ zkClient.writeData(path, "v2");
+
+ Assert.assertTrue(countDownLatch.await(3L, TimeUnit.SECONDS));
Review comment:
3L is 3 milli second? This can be a source of test unstableness. Let us try 3 second. Using 3 second does not really mean you will wait 3 second anyway.
nit: Also, define the waiting period using a constant static.
----------------------------------------------------------------
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] lei-xia commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r479586535
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,19 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change.
+ */
Review comment:
Can we add more description here, such as if this is disabled, in the ZkClient's callback handleDataChange(Path, Data), you should expect the data is null. Also, mention what is the default value if not set.
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1332: Fix PreFetch annotation in ZkClient and CallbackHandler
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1332:
URL: https://github.com/apache/helix/pull/1332#discussion_r480375132
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/annotation/PreFetchChangedData.java
##########
@@ -22,8 +22,25 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
-
+/**
+ * An annotation used to prefetch changed data for
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener} in ZkClient.
+ * By default, prefetch is enabled: when ZkClient handles a data change event,
+ * ZkClient will read data and pass data object to
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}.
+ * If disabled({@code false}): ZkClient will not read data, so data object is passed as null.
+ * <p>
+ * Example:
+ * <pre>
+ * {@code @PreFetch(enabled = false)}
+ * public class MyCallback implements IZkDataListener
+ * </pre>
+ *
+ * {@code @PreFetch(enabled = false)} means data will not be prefetched in ZkClient before
+ * handling data change: data is null in
+ * {@link org.apache.helix.zookeeper.zkclient.IZkDataListener#handleDataChange(String, Object)}
+ */
@Retention(RetentionPolicy.RUNTIME)
Review comment:
What is the use for this @retention ?
----------------------------------------------------------------
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