You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "xyuanlu (via GitHub)" <gi...@apache.org> on 2023/01/27 22:41:16 UTC
[GitHub] [helix] xyuanlu opened a new pull request, #2354: [WIP] Implement zk Meta client async crud
xyuanlu opened a new pull request, #2354:
URL: https://github.com/apache/helix/pull/2354
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1113919539
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClientAsyncOperations.java:
##########
@@ -0,0 +1,231 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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 javax.annotation.Nullable;
+
+import org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.zookeeper.KeeperException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZkMetaClientAsyncOperations extends ZkMetaClientTestBase {
+
+ static TestAsyncContext[] asyncContext = new TestAsyncContext[1];
+ static final String entryKey = "/TestAsyncEntryKey";
+ static final String nonExistsEntry = "/a/b/c";
+
+ static class TestAsyncContext {
+ int _asyncCallSize;
+ CountDownLatch _countDownLatch;
+ int[] _returnCode;
+ MetaClientInterface.Stat[] _stats;
+ String[] _data;
+
+ TestAsyncContext(int callSize) {
+ _asyncCallSize = callSize;
+ _countDownLatch = new CountDownLatch(callSize);
+ _returnCode = new int[callSize];
+ _stats = new MetaClientInterface.Stat[callSize];
+ _data = new String[callSize];
+ }
+
+ public CountDownLatch getCountDownLatch() {
+ return _countDownLatch;
+ }
+
+ public void countDown() {
+ _countDownLatch.countDown();
+ }
+
+ public int getReturnCode(int idx) {
+ return _returnCode[idx];
+ }
+
+ public MetaClientInterface.Stat getStats(int idx) {
+ return _stats[idx];
+ }
+
+ public String getData(int idx) {
+ return _data[idx];
+ }
+
+ public void setReturnCodeWhenFinished(int idx, int returnCode) {
+ _returnCode[idx] = returnCode;
+ }
+
+ public void setStatWhenFinished(int idx, MetaClientInterface.Stat stat) {
+ _stats[idx] = stat;
+ }
+
+ public void setDataWhenFinished(int idx, String data) {
+ _data[idx] = data;
+ }
+ }
+
+ @Test
+ public void testAsyncCreateSetAndGet() {
+ asyncContext[0] = new TestAsyncContext(2);
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ zkMetaClient.connect();
+
+ zkMetaClient
+ .asyncCreate(entryKey, "async_create-data", MetaClientInterface.EntryMode.PERSISTENT,
+ new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+
+ zkMetaClient.asyncCreate(nonExistsEntry, "async_create-data-invalid",
+ MetaClientInterface.EntryMode.PERSISTENT, new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(1, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+
+ asyncContext[0].getCountDownLatch().await();
+
+ Assert.assertEquals(asyncContext[0].getReturnCode(0), KeeperException.Code.OK.intValue());
+ Assert.assertEquals(asyncContext[0].getReturnCode(1), KeeperException.Code.NONODE.intValue());
+
+ // create the entry again and expect a duplicated error code
+ asyncContext[0] = new TestAsyncContext(1);
+ zkMetaClient
+ .asyncCreate(entryKey, "async_create-data", MetaClientInterface.EntryMode.PERSISTENT,
+ new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+ asyncContext[0].getCountDownLatch().await();
+ Assert.assertEquals(asyncContext[0].getReturnCode(0),
+ KeeperException.Code.NODEEXISTS.intValue());
+
+
+ // test set
+ asyncContext[0] = new TestAsyncContext(1);
+ zkMetaClient
+ .asyncSet(entryKey, "async_create-data-new", 0,
+ new AsyncCallback.StatCallback() {
+ @Override
+ public void processResult(int returnCode, String key,
+ @Nullable MetaClientInterface.Stat stat) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].setStatWhenFinished(0, stat);
+ asyncContext[0].countDown();
+ }
+ });
+ asyncContext[0].getCountDownLatch().await();
Review Comment:
TFTR. Updated.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] rahulrane50 commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1110435826
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClientAsyncOperations.java:
##########
@@ -0,0 +1,231 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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 javax.annotation.Nullable;
+
+import org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.zookeeper.KeeperException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZkMetaClientAsyncOperations extends ZkMetaClientTestBase {
+
+ static TestAsyncContext[] asyncContext = new TestAsyncContext[1];
+ static final String entryKey = "/TestAsyncEntryKey";
+ static final String nonExistsEntry = "/a/b/c";
+
+ static class TestAsyncContext {
+ int _asyncCallSize;
+ CountDownLatch _countDownLatch;
+ int[] _returnCode;
+ MetaClientInterface.Stat[] _stats;
+ String[] _data;
+
+ TestAsyncContext(int callSize) {
+ _asyncCallSize = callSize;
+ _countDownLatch = new CountDownLatch(callSize);
+ _returnCode = new int[callSize];
+ _stats = new MetaClientInterface.Stat[callSize];
+ _data = new String[callSize];
+ }
+
+ public CountDownLatch getCountDownLatch() {
+ return _countDownLatch;
+ }
+
+ public void countDown() {
+ _countDownLatch.countDown();
+ }
+
+ public int getReturnCode(int idx) {
+ return _returnCode[idx];
+ }
+
+ public MetaClientInterface.Stat getStats(int idx) {
+ return _stats[idx];
+ }
+
+ public String getData(int idx) {
+ return _data[idx];
+ }
+
+ public void setReturnCodeWhenFinished(int idx, int returnCode) {
+ _returnCode[idx] = returnCode;
+ }
+
+ public void setStatWhenFinished(int idx, MetaClientInterface.Stat stat) {
+ _stats[idx] = stat;
+ }
+
+ public void setDataWhenFinished(int idx, String data) {
+ _data[idx] = data;
+ }
+ }
+
+ @Test
+ public void testAsyncCreateSetAndGet() {
+ asyncContext[0] = new TestAsyncContext(2);
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
Review Comment:
Neat : Excellent call on using try-with-resources here. I feel most of our unstable UTs/tests are due to this non-closed zkclients which are being shared across multiple tests.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] qqu0127 commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "qqu0127 (via GitHub)" <gi...@apache.org>.
qqu0127 commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1110440102
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -515,5 +515,9 @@ default boolean subscribeOneTimeChildChanges(String key, ChildChangeListener lis
*/
boolean waitUntilExists(String key, TimeUnit timeUnit, long timeOut);
+ public byte[] serialize(T data, String path);
+
+ public T deserialize(byte[] bytes, String path);
Review Comment:
+1 on documentation. Why do we need these two methods?
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientTestBase.java:
##########
@@ -0,0 +1,95 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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.io.File;
+import java.io.IOException;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
+import org.apache.helix.zookeeper.zkclient.IDefaultNameSpace;
+import org.apache.helix.zookeeper.zkclient.ZkServer;
+import org.testng.annotations.AfterSuite;
+import org.testng.annotations.BeforeSuite;
+
+
+public abstract class ZkMetaClientTestBase {
+
+ protected static final String ZK_ADDR = "localhost:2183";
+ protected static final int DEFAULT_TIMEOUT_MS = 1000;
+ protected static final String ENTRY_STRING_VALUE = "test-value";
+
+ private final Object _syncObject = new Object();
Review Comment:
not used
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/ZkMetaClientTestBase.java:
##########
@@ -0,0 +1,95 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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.io.File;
+import java.io.IOException;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
+import org.apache.helix.zookeeper.zkclient.IDefaultNameSpace;
+import org.apache.helix.zookeeper.zkclient.ZkServer;
+import org.testng.annotations.AfterSuite;
+import org.testng.annotations.BeforeSuite;
+
+
+public abstract class ZkMetaClientTestBase {
+
+ protected static final String ZK_ADDR = "localhost:2183";
+ protected static final int DEFAULT_TIMEOUT_MS = 1000;
+ protected static final String ENTRY_STRING_VALUE = "test-value";
+
+ private final Object _syncObject = new Object();
+
+ private ZkServer _zkServer;
Review Comment:
An open discussion here. So with BeforeSuite, testng will only trigger that method once. That means for most subclasses of this, this `_zkServer` will be null. Maybe we should set this as static? But I'm really not sure if this is the right approach.
(And I don't know what will happen with afterSuite, could there be NPE?)
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClientAsyncOperations.java:
##########
@@ -0,0 +1,231 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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 javax.annotation.Nullable;
+
+import org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.zookeeper.KeeperException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZkMetaClientAsyncOperations extends ZkMetaClientTestBase {
+
+ static TestAsyncContext[] asyncContext = new TestAsyncContext[1];
+ static final String entryKey = "/TestAsyncEntryKey";
+ static final String nonExistsEntry = "/a/b/c";
+
+ static class TestAsyncContext {
+ int _asyncCallSize;
+ CountDownLatch _countDownLatch;
+ int[] _returnCode;
+ MetaClientInterface.Stat[] _stats;
+ String[] _data;
+
+ TestAsyncContext(int callSize) {
+ _asyncCallSize = callSize;
+ _countDownLatch = new CountDownLatch(callSize);
+ _returnCode = new int[callSize];
+ _stats = new MetaClientInterface.Stat[callSize];
+ _data = new String[callSize];
+ }
+
+ public CountDownLatch getCountDownLatch() {
+ return _countDownLatch;
+ }
+
+ public void countDown() {
+ _countDownLatch.countDown();
+ }
+
+ public int getReturnCode(int idx) {
+ return _returnCode[idx];
+ }
+
+ public MetaClientInterface.Stat getStats(int idx) {
+ return _stats[idx];
+ }
+
+ public String getData(int idx) {
+ return _data[idx];
+ }
+
+ public void setReturnCodeWhenFinished(int idx, int returnCode) {
+ _returnCode[idx] = returnCode;
+ }
+
+ public void setStatWhenFinished(int idx, MetaClientInterface.Stat stat) {
+ _stats[idx] = stat;
+ }
+
+ public void setDataWhenFinished(int idx, String data) {
+ _data[idx] = data;
+ }
+ }
+
+ @Test
+ public void testAsyncCreateSetAndGet() {
+ asyncContext[0] = new TestAsyncContext(2);
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ zkMetaClient.connect();
+
+ zkMetaClient
+ .asyncCreate(entryKey, "async_create-data", MetaClientInterface.EntryMode.PERSISTENT,
+ new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+
+ zkMetaClient.asyncCreate(nonExistsEntry, "async_create-data-invalid",
+ MetaClientInterface.EntryMode.PERSISTENT, new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(1, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+
+ asyncContext[0].getCountDownLatch().await();
+
+ Assert.assertEquals(asyncContext[0].getReturnCode(0), KeeperException.Code.OK.intValue());
+ Assert.assertEquals(asyncContext[0].getReturnCode(1), KeeperException.Code.NONODE.intValue());
+
+ // create the entry again and expect a duplicated error code
+ asyncContext[0] = new TestAsyncContext(1);
+ zkMetaClient
+ .asyncCreate(entryKey, "async_create-data", MetaClientInterface.EntryMode.PERSISTENT,
+ new AsyncCallback.VoidCallback() {
+ @Override
+ public void processResult(int returnCode, String key) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].countDown();
+ }
+ });
+ asyncContext[0].getCountDownLatch().await();
+ Assert.assertEquals(asyncContext[0].getReturnCode(0),
+ KeeperException.Code.NODEEXISTS.intValue());
+
+
+ // test set
+ asyncContext[0] = new TestAsyncContext(1);
+ zkMetaClient
+ .asyncSet(entryKey, "async_create-data-new", 0,
+ new AsyncCallback.StatCallback() {
+ @Override
+ public void processResult(int returnCode, String key,
+ @Nullable MetaClientInterface.Stat stat) {
+ asyncContext[0].setReturnCodeWhenFinished(0, returnCode);
+ asyncContext[0].setStatWhenFinished(0, stat);
+ asyncContext[0].countDown();
+ }
+ });
+ asyncContext[0].getCountDownLatch().await();
Review Comment:
nit: better to set a timeout and Assert on that
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] rahulrane50 commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1110430808
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/ZkMetaClientSetCallbackHandler.java:
##########
@@ -0,0 +1,42 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * 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 org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks;
+
+
+public class ZkMetaClientSetCallbackHandler extends ZkAsyncCallbacks.SetDataCallbackHandler {
+ AsyncCallback.StatCallback userCallback;
+
+ public ZkMetaClientSetCallbackHandler(AsyncCallback.StatCallback cb) {
+ userCallback = cb;
+ }
+
+ @Override
+ public void handle() {
+ userCallback.processResult(getRc(), getPath(), getStat() == null ? null
Review Comment:
Neat : there are multiple getStat calls in this line. May be we can rearrange the code and call use few local variables to remove few duplicate 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1113920232
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClientAsyncOperations.java:
##########
@@ -0,0 +1,231 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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 javax.annotation.Nullable;
+
+import org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.zookeeper.KeeperException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZkMetaClientAsyncOperations extends ZkMetaClientTestBase {
+
+ static TestAsyncContext[] asyncContext = new TestAsyncContext[1];
+ static final String entryKey = "/TestAsyncEntryKey";
+ static final String nonExistsEntry = "/a/b/c";
+
+ static class TestAsyncContext {
+ int _asyncCallSize;
Review Comment:
I feel like it is more of a common pattern to use getter and setter. Although not strict in test code.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1113920430
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -515,5 +515,9 @@ default boolean subscribeOneTimeChildChanges(String key, ChildChangeListener lis
*/
boolean waitUntilExists(String key, TimeUnit timeUnit, long timeOut);
+ public byte[] serialize(T data, String path);
Review Comment:
TFTR. Updated.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] rahulrane50 commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1110428011
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -515,5 +515,9 @@ default boolean subscribeOneTimeChildChanges(String key, ChildChangeListener lis
*/
boolean waitUntilExists(String key, TimeUnit timeUnit, long timeOut);
+ public byte[] serialize(T data, String path);
Review Comment:
Neat : do you think API documentation can be added here just like other APIs in this interface?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1113920946
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/ZkMetaClientSetCallbackHandler.java:
##########
@@ -0,0 +1,42 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * 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 org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks;
+
+
+public class ZkMetaClientSetCallbackHandler extends ZkAsyncCallbacks.SetDataCallbackHandler {
+ AsyncCallback.StatCallback userCallback;
+
+ public ZkMetaClientSetCallbackHandler(AsyncCallback.StatCallback cb) {
+ userCallback = cb;
+ }
+
+ @Override
+ public void handle() {
+ userCallback.processResult(getRc(), getPath(), getStat() == null ? null
Review Comment:
I feel like having a local var won't change a lot. As the getter function itself is short and straight forward.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] rahulrane50 commented on a diff in pull request #2354: Implement zk Meta client async crud
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2354:
URL: https://github.com/apache/helix/pull/2354#discussion_r1110433711
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClientAsyncOperations.java:
##########
@@ -0,0 +1,231 @@
+package org.apache.helix.metaclient.impl.zk;
+
+/*
+ * 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 javax.annotation.Nullable;
+
+import org.apache.helix.metaclient.api.AsyncCallback;
+import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.zookeeper.KeeperException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZkMetaClientAsyncOperations extends ZkMetaClientTestBase {
+
+ static TestAsyncContext[] asyncContext = new TestAsyncContext[1];
+ static final String entryKey = "/TestAsyncEntryKey";
+ static final String nonExistsEntry = "/a/b/c";
+
+ static class TestAsyncContext {
+ int _asyncCallSize;
Review Comment:
Curious, since you are keeping all these members as "default" scope do we still need all getter methods?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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] xyuanlu merged pull request #2354: Implement zk Meta client async crud
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2354:
URL: https://github.com/apache/helix/pull/2354
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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