[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380867519
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
Review comment:
This is addressing the previous ZkClientConfig "duplicate code" issue,
right? What is the reason that previous it does not work?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
narendly commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380874975
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
Review comment:
As I explained, the issue was trying to
> cast superclass to subclass which is not allowed
I had to override so that these setters return an instance of
HelixZkClient.ZkClientConfig instead of RealmAwareZkClientConfig. I suggest
thinking more carefully about the return types and their inheritance
relationships.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380875510
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -125,4 +132,74 @@ public int getActiveConnectionCount() {
}
return count;
}
+
+ public interface OnCloseCallback {
+/**
+ * Triggered after the SharedZkClient is closed.
+ */
+void onClose();
+ }
+
+ /**
+ * NOTE: do NOT use this class directly. Please use SharedZkClientFactory to
create an instance of SharedZkClient.
+ * InnerSharedZkClient is a ZkClient used by SharedZkClient to power ZK
operations against a single ZK realm.
+ */
+ public static class InnerSharedZkClient extends ZkClient implements
HelixZkClient {
Review comment:
So basically InnerSharedZkClient is exactly the original SharedZkClient. Is
the code exactly the same?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
narendly commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380875012
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
+@Override
Review comment:
As I explained, the issue was trying to
> cast superclass to subclass which is not allowed
I had to override so that these setters return an instance of
HelixZkClient.ZkClientConfig instead of RealmAwareZkClientConfig. I suggest
thinking more carefully about the return types and their inheritance
relationships.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380882410
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -125,4 +132,74 @@ public int getActiveConnectionCount() {
}
return count;
}
+
+ public interface OnCloseCallback {
+/**
+ * Triggered after the SharedZkClient is closed.
+ */
+void onClose();
+ }
+
+ /**
+ * NOTE: do NOT use this class directly. Please use SharedZkClientFactory to
create an instance of SharedZkClient.
+ * InnerSharedZkClient is a ZkClient used by SharedZkClient to power ZK
operations against a single ZK realm.
+ */
+ public static class InnerSharedZkClient extends ZkClient implements
HelixZkClient {
Review comment:
This is actually what I worried, the code here changed. How do we reason
that the new implementation of InnerSharedZkClient keeps the old
InnerSharedZkClient original behavior?
@jiajunwang can you have a look?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
narendly commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380862823
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/TrieRoutingData.java
##
@@ -0,0 +1,277 @@
+package org.apache.helix.zookeeper.impl.factory;
+
+/*
+ * 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.ArrayDeque;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.NoSuchElementException;
+
+
+/**
+ * TODO: remove when there's a separate module.
+ * This is a class that uses a data structure similar to trie to represent
metadata store routing
+ * data. It is not exactly a trie because it in essence stores a mapping (from
sharding keys to
+ * realm addresses) instead of pure text information; also, only the terminal
nodes store meaningful
+ * information (realm addresses).
+ */
+public class TrieRoutingData implements MetadataStoreRoutingData {
Review comment:
This class will be part of metadata-store-directory-common module.
Once https://github.com/apache/helix/pull/771 is checked in, this class will
be removed as noted in the JavaDoc.
We cannot directly use this class because that would create a circular
dependency.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
narendly commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380862823
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/TrieRoutingData.java
##
@@ -0,0 +1,277 @@
+package org.apache.helix.zookeeper.impl.factory;
+
+/*
+ * 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.ArrayDeque;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.NoSuchElementException;
+
+
+/**
+ * TODO: remove when there's a separate module.
+ * This is a class that uses a data structure similar to trie to represent
metadata store routing
+ * data. It is not exactly a trie because it in essence stores a mapping (from
sharding keys to
+ * realm addresses) instead of pure text information; also, only the terminal
nodes store meaningful
+ * information (realm addresses).
+ */
+public class TrieRoutingData implements MetadataStoreRoutingData {
Review comment:
This class will be part of metadata-store-directory-common module.
Once https://github.com/apache/helix/pull/771 is checked in, this class will
be removed.
We cannot directly use this class because that would create a circular
dependency.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380867519
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
Review comment:
This is the addressing the previous ZkClientConfig "duplicate code" issue,
right? What is the reason that previous it does not work.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380867519
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
Review comment:
This is the addressing the previous ZkClientConfig "duplicate code" issue,
right? What is the reason that previous it does not work?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380868468
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -89,32 +88,17 @@ public int getSessionTimeout() {
* Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
*
* Configuration for creating a new HelixZkClient with serializer and
monitor.
- *
- * TODO: If possible, try to merge with RealmAwareZkClient's
RealmAwareZkClientConfig to reduce duplicate logic/code (without breaking
backward-compatibility).
- * Simply making this a subclass of RealmAwareZkClientConfig will break
backward-compatiblity.
*/
@Deprecated
- class ZkClientConfig {
-// For client to init the connection
-private long _connectInitTimeout = DEFAULT_CONNECTION_TIMEOUT;
-
-// Data access configs
-private long _operationRetryTimeout = DEFAULT_OPERATION_TIMEOUT;
-
-// Others
-private PathBasedZkSerializer _zkSerializer;
-
-// Monitoring
-private String _monitorType;
-private String _monitorKey;
-private String _monitorInstanceName = null;
-private boolean _monitorRootPathOnly = true;
+ class ZkClientConfig extends RealmAwareZkClientConfig {
+@Override
Review comment:
Why do we need to re-implement these methods if you inherit from the
RealmAwareZkClientConfig?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380916271
##
File path:
helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
##
@@ -0,0 +1,145 @@
+/*
+ * 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.
+ */
+
+package org.apache.helix.lock.helix;
+
+import java.util.Date;
+
+import org.I0Itec.zkclient.DataUpdater;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.lock.HelixLock;
+import org.apache.helix.lock.LockInfo;
+import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.log4j.Logger;
+
+
+/**
+ * Helix nonblocking lock implementation based on Zookeeper
+ */
+public class ZKHelixNonblockingLock implements HelixLock {
+
+ private static final Logger LOG =
Logger.getLogger(ZKHelixNonblockingLock.class);
+
+ private final String _lockPath;
+ private final String _userId;
+ private final long _timeout;
+ private final String _lockMsg;
+ private final BaseDataAccessor _baseDataAccessor;
+
+ /**
+ * Initialize the lock with user provided information, e.g.,cluster, scope,
etc.
+ * @param scope the scope to lock
+ * @param zkAddress the zk address the cluster connects to
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ public ZKHelixNonblockingLock(LockScope scope, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+this(scope.getPath(), zkAddress, timeout, lockMsg, userId);
+ }
+
+ /**
+ * Initialize the lock with user provided information, e.g., lock path under
zookeeper, etc.
+ * @param lockPath the path of the lock under Zookeeper
+ * @param zkAddress the zk address of the cluster
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ private ZKHelixNonblockingLock(String lockPath, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+_lockPath = lockPath;
+if (timeout < 0) {
+ throw new IllegalArgumentException("The expiration time cannot be
negative.");
+}
+_timeout = timeout;
+_lockMsg = lockMsg;
+_userId = userId;
+_baseDataAccessor = new ZkBaseDataAccessor<>(zkAddress);
+ }
+
+ @Override
+ public boolean acquireLock() {
+
+// Set lock information fields
+long deadline;
+// Prevent value overflow
+if (_timeout > Long.MAX_VALUE - System.currentTimeMillis()) {
+ deadline = Long.MAX_VALUE;
+} else {
+ deadline = System.currentTimeMillis() + _timeout;
+}
+LockUpdater updater = new LockUpdater(new LockInfo(_userId, _lockMsg,
deadline));
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public boolean releaseLock() {
+// Initialize the lock updater with a default lock info represents the
state of a unlocked lock
+LockUpdater updater = new LockUpdater(LockInfo.defaultLockInfo);
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
Review comment:
Why we dont delete the node instead of updating 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380915772
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,109 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo extends HelixProperty {
Review comment:
Put a TODO here. This Helix Property will be replaced since distributed lock
will be a module independent from Helix.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380916833
##
File path:
helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
##
@@ -0,0 +1,145 @@
+/*
+ * 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.
+ */
+
+package org.apache.helix.lock.helix;
+
+import java.util.Date;
+
+import org.I0Itec.zkclient.DataUpdater;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.lock.HelixLock;
+import org.apache.helix.lock.LockInfo;
+import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.log4j.Logger;
+
+
+/**
+ * Helix nonblocking lock implementation based on Zookeeper
+ */
+public class ZKHelixNonblockingLock implements HelixLock {
+
+ private static final Logger LOG =
Logger.getLogger(ZKHelixNonblockingLock.class);
+
+ private final String _lockPath;
+ private final String _userId;
+ private final long _timeout;
+ private final String _lockMsg;
+ private final BaseDataAccessor _baseDataAccessor;
+
+ /**
+ * Initialize the lock with user provided information, e.g.,cluster, scope,
etc.
+ * @param scope the scope to lock
+ * @param zkAddress the zk address the cluster connects to
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ public ZKHelixNonblockingLock(LockScope scope, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+this(scope.getPath(), zkAddress, timeout, lockMsg, userId);
+ }
+
+ /**
+ * Initialize the lock with user provided information, e.g., lock path under
zookeeper, etc.
+ * @param lockPath the path of the lock under Zookeeper
+ * @param zkAddress the zk address of the cluster
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ private ZKHelixNonblockingLock(String lockPath, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+_lockPath = lockPath;
+if (timeout < 0) {
+ throw new IllegalArgumentException("The expiration time cannot be
negative.");
+}
+_timeout = timeout;
+_lockMsg = lockMsg;
+_userId = userId;
+_baseDataAccessor = new ZkBaseDataAccessor<>(zkAddress);
+ }
+
+ @Override
+ public boolean acquireLock() {
+
+// Set lock information fields
+long deadline;
+// Prevent value overflow
+if (_timeout > Long.MAX_VALUE - System.currentTimeMillis()) {
+ deadline = Long.MAX_VALUE;
+} else {
+ deadline = System.currentTimeMillis() + _timeout;
+}
+LockUpdater updater = new LockUpdater(new LockInfo(_userId, _lockMsg,
deadline));
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public boolean releaseLock() {
+// Initialize the lock updater with a default lock info represents the
state of a unlocked lock
+LockUpdater updater = new LockUpdater(LockInfo.defaultLockInfo);
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public LockInfo getCurrentLockInfo() {
+ZNRecord curLockInfo = _baseDataAccessor.get(_lockPath, null,
AccessOption.PERSISTENT);
+return new LockInfo(curLockInfo);
Review comment:
What if the path does not exists?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #776: add CustomizedStateAggregation config
alirezazamani commented on a change in pull request #776: add
CustomizedStateAggregation config
URL: https://github.com/apache/helix/pull/776#discussion_r380914433
##
File path:
helix-core/src/main/java/org/apache/helix/model/CustomizedStateAggregationConfig.java
##
@@ -0,0 +1,144 @@
+package org.apache.helix.model;
+
+/*
+ * 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.ArrayList;
+import java.util.List;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
+
+/**
+ * CustomizedStateAggregation configurations
+ */
+public class CustomizedStateAggregationConfig extends HelixProperty {
+ /**
+ * Indicate which customized states will be aggregated.
+ * NOTE: Do NOT use this field name directly, use its corresponding
getter/setter in the
+ * CustomizedStateAggregationConfig.
+ */
+ public enum CustomizedStateAggregationProperty {
+AGGREGATION_ENABLED_STATES,
+ }
+
+ /**
+ * Instantiate the CustomizedStateAggregationConfig
+ * @param cluster
+ */
+ public CustomizedStateAggregationConfig(String cluster) {
+super(cluster);
+ }
+
+ /**
+ * Instantiate with a pre-populated record
+ * @param record a ZNRecord corresponding to a
CustomizedStateAggregationConfig
+ */
+ public CustomizedStateAggregationConfig(ZNRecord record) {
+super(record);
+ }
+
+ /**
+ * Instantiate the config using each field individually.
+ * Users should use CustomizedStateAggregationConfig.Builder to create
+ * CustomizedStateAggregationConfig.
+ * @param cluster
+ * @param aggregationEnabledStates
+ */
+ public CustomizedStateAggregationConfig(String cluster, List
aggregationEnabledStates) {
+super(cluster);
+
_record.setListField(CustomizedStateAggregationProperty.AGGREGATION_ENABLED_STATES.name(),
+aggregationEnabledStates);
+
+ }
+
+ /**
+ * Set the AGGREGATION_ENABLED_STATES field.
+ * @param aggregationEnabledStates
+ */
+ public void setAggregationEnabledStates(List
aggregationEnabledStates) {
+
_record.setListField(CustomizedStateAggregationProperty.AGGREGATION_ENABLED_STATES.name(),
+aggregationEnabledStates);
+ }
+
+ /**
+ * Get the AGGREGATION_ENABLED_STATES field.
+ * @return AGGREGATION_ENABLED_STATES field.
+ */
+ public List getAggregationEnabledStates() {
+return _record
+
.getListField(CustomizedStateAggregationProperty.AGGREGATION_ENABLED_STATES.name());
+ }
+
+ public static class Builder {
+private String _clusterName = null;
+private List _aggregationEnabledStates;
Review comment:
If we decided to go with AGGREGATION_ENABLED_TYPE, please change this name
as well.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #776: add CustomizedStateAggregation config
alirezazamani commented on a change in pull request #776: add
CustomizedStateAggregation config
URL: https://github.com/apache/helix/pull/776#discussion_r380913969
##
File path:
helix-core/src/main/java/org/apache/helix/model/CustomizedStateAggregationConfig.java
##
@@ -0,0 +1,144 @@
+package org.apache.helix.model;
+
+/*
+ * 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.ArrayList;
+import java.util.List;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
+
+/**
+ * CustomizedStateAggregation configurations
+ */
+public class CustomizedStateAggregationConfig extends HelixProperty {
+ /**
+ * Indicate which customized states will be aggregated.
+ * NOTE: Do NOT use this field name directly, use its corresponding
getter/setter in the
+ * CustomizedStateAggregationConfig.
+ */
+ public enum CustomizedStateAggregationProperty {
+AGGREGATION_ENABLED_STATES,
Review comment:
can we have AGGREGATION_ENABLED_TYPES instead?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #776: add CustomizedStateAggregation config
zhangmeng916 commented on a change in pull request #776: add
CustomizedStateAggregation config
URL: https://github.com/apache/helix/pull/776#discussion_r380952867
##
File path:
helix-core/src/main/java/org/apache/helix/model/CustomizedStateAggregationConfig.java
##
@@ -0,0 +1,144 @@
+package org.apache.helix.model;
+
+/*
+ * 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.ArrayList;
+import java.util.List;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
+
+/**
+ * CustomizedStateAggregation configurations
+ */
+public class CustomizedStateAggregationConfig extends HelixProperty {
+ /**
+ * Indicate which customized states will be aggregated.
+ * NOTE: Do NOT use this field name directly, use its corresponding
getter/setter in the
+ * CustomizedStateAggregationConfig.
+ */
+ public enum CustomizedStateAggregationProperty {
+AGGREGATION_ENABLED_STATES,
Review comment:
My thinking is that this field include all "states" names that shall be
aggregated. I did not have "types" concept defined anywhere. Please let me know
if need further discussion.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
narendly commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380935789
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -125,4 +132,74 @@ public int getActiveConnectionCount() {
}
return count;
}
+
+ public interface OnCloseCallback {
+/**
+ * Triggered after the SharedZkClient is closed.
+ */
+void onClose();
+ }
+
+ /**
+ * NOTE: do NOT use this class directly. Please use SharedZkClientFactory to
create an instance of SharedZkClient.
+ * InnerSharedZkClient is a ZkClient used by SharedZkClient to power ZK
operations against a single ZK realm.
+ */
+ public static class InnerSharedZkClient extends ZkClient implements
HelixZkClient {
Review comment:
@kaisun2000
InnerSharedZkClient is meant to preserve the behavior/logic of the original
SharedZkClient.
1. Clearly the code is not "exactly" the same. You have access to the
original SharedZkClient - please do a comparison.
2. What is the exact concern here? I'm not exactly sure what your question
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #718: Implement Helix nonblocking lock
mgao0 commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380960097
##
File path:
helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
##
@@ -0,0 +1,145 @@
+/*
+ * 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.
+ */
+
+package org.apache.helix.lock.helix;
+
+import java.util.Date;
+
+import org.I0Itec.zkclient.DataUpdater;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.lock.HelixLock;
+import org.apache.helix.lock.LockInfo;
+import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.log4j.Logger;
+
+
+/**
+ * Helix nonblocking lock implementation based on Zookeeper
+ */
+public class ZKHelixNonblockingLock implements HelixLock {
+
+ private static final Logger LOG =
Logger.getLogger(ZKHelixNonblockingLock.class);
+
+ private final String _lockPath;
+ private final String _userId;
+ private final long _timeout;
+ private final String _lockMsg;
+ private final BaseDataAccessor _baseDataAccessor;
+
+ /**
+ * Initialize the lock with user provided information, e.g.,cluster, scope,
etc.
+ * @param scope the scope to lock
+ * @param zkAddress the zk address the cluster connects to
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ public ZKHelixNonblockingLock(LockScope scope, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+this(scope.getPath(), zkAddress, timeout, lockMsg, userId);
+ }
+
+ /**
+ * Initialize the lock with user provided information, e.g., lock path under
zookeeper, etc.
+ * @param lockPath the path of the lock under Zookeeper
+ * @param zkAddress the zk address of the cluster
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ private ZKHelixNonblockingLock(String lockPath, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+_lockPath = lockPath;
+if (timeout < 0) {
+ throw new IllegalArgumentException("The expiration time cannot be
negative.");
+}
+_timeout = timeout;
+_lockMsg = lockMsg;
+_userId = userId;
+_baseDataAccessor = new ZkBaseDataAccessor<>(zkAddress);
+ }
+
+ @Override
+ public boolean acquireLock() {
+
+// Set lock information fields
+long deadline;
+// Prevent value overflow
+if (_timeout > Long.MAX_VALUE - System.currentTimeMillis()) {
+ deadline = Long.MAX_VALUE;
+} else {
+ deadline = System.currentTimeMillis() + _timeout;
+}
+LockUpdater updater = new LockUpdater(new LockInfo(_userId, _lockMsg,
deadline));
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public boolean releaseLock() {
+// Initialize the lock updater with a default lock info represents the
state of a unlocked lock
+LockUpdater updater = new LockUpdater(LockInfo.defaultLockInfo);
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public LockInfo getCurrentLockInfo() {
+ZNRecord curLockInfo = _baseDataAccessor.get(_lockPath, null,
AccessOption.PERSISTENT);
+return new LockInfo(curLockInfo);
Review comment:
Data accessor returns a null, and when converting it to the LockInfo, a
LockInfo instance with all the fields filled with default values (which is also
the value we use to represent an unlocked lock node) will be returned.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380959695
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -125,4 +132,74 @@ public int getActiveConnectionCount() {
}
return count;
}
+
+ public interface OnCloseCallback {
+/**
+ * Triggered after the SharedZkClient is closed.
+ */
+void onClose();
+ }
+
+ /**
+ * NOTE: do NOT use this class directly. Please use SharedZkClientFactory to
create an instance of SharedZkClient.
+ * InnerSharedZkClient is a ZkClient used by SharedZkClient to power ZK
operations against a single ZK realm.
+ */
+ public static class InnerSharedZkClient extends ZkClient implements
HelixZkClient {
Review comment:
I had a careful look, this seems to be OK to maintain the original behavior.
Assertion 1.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
narendly commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380862050
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -44,15 +44,17 @@ protected SharedZkClientFactory() {
@Override
public RealmAwareZkClient buildZkClient(
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
- RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
Review comment:
There was a change in the interface. SharedZkClient implements that
interface. So this change is just a signature change, not logical change.
This change is necessary.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
dasahcc commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380857248
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -44,15 +44,17 @@ protected SharedZkClientFactory() {
@Override
public RealmAwareZkClient buildZkClient(
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
- RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
Review comment:
This is PR for dedicated zkclient factory, 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
dasahcc commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380858300
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/TrieRoutingData.java
##
@@ -0,0 +1,277 @@
+package org.apache.helix.zookeeper.impl.factory;
+
+/*
+ * 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.ArrayDeque;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.NoSuchElementException;
+
+
+/**
+ * TODO: remove when there's a separate module.
+ * This is a class that uses a data structure similar to trie to represent
metadata store routing
+ * data. It is not exactly a trie because it in essence stores a mapping (from
sharding keys to
+ * realm addresses) instead of pure text information; also, only the terminal
nodes store meaningful
+ * information (realm addresses).
+ */
+public class TrieRoutingData implements MetadataStoreRoutingData {
Review comment:
Is this new code or refactor? If refactor, we should remove the old code in
REST module, 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly opened a new issue #777: Create rest-common module for all REST services
narendly opened a new issue #777: Create rest-common module for all REST services URL: https://github.com/apache/helix/issues/777 Since there will be two services that will be deployed in a RESTful way, we could refactor the common JAX-RS rest startable logic out into a common module called rest-common. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380961009
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
##
@@ -37,79 +50,541 @@
* HelixZkClient that uses shared ZkConnection.
* A SharedZkClient won't manipulate the shared ZkConnection directly.
*/
-public class SharedZkClient extends ZkClient implements HelixZkClient {
+public class SharedZkClient implements RealmAwareZkClient {
private static Logger LOG = LoggerFactory.getLogger(SharedZkClient.class);
- /*
- * Since we cannot really disconnect the ZkConnection, we need a dummy
ZkConnection placeholder.
- * This is to ensure connection field is never null even the shared
RealmAwareZkClient instance is closed so as to avoid NPE.
- */
- private final static ZkConnection IDLE_CONNECTION = new
ZkConnection("Dummy_ZkServers");
- private final OnCloseCallback _onCloseCallback;
- private final ZkConnectionManager _connectionManager;
-
- public interface OnCloseCallback {
-/**
- * Triggered after the RealmAwareZkClient is closed.
- */
-void onClose();
- }
-
- /**
- * Construct a shared RealmAwareZkClient that uses a shared ZkConnection.
- *
- * @param connectionManager The manager of the shared ZkConnection.
- * @param clientConfig ZkClientConfig details to create the shared
RealmAwareZkClient.
- * @param callback Clean up logic when the shared
RealmAwareZkClient is closed.
- */
- public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig
clientConfig,
- OnCloseCallback callback) {
-super(connectionManager.getConnection(), 0,
clientConfig.getOperationRetryTimeout(),
-clientConfig.getZkSerializer(), clientConfig.getMonitorType(),
clientConfig.getMonitorKey(),
-clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
-_connectionManager = connectionManager;
-// Register to the base dedicated RealmAwareZkClient
-_connectionManager.registerWatcher(this);
-_onCloseCallback = callback;
+
+ private final HelixZkClient _innerSharedZkClient;
+ private final Map _routingDataCache; // TODO: replace with
RoutingDataCache
+ private final String _zkRealmShardingKey;
+
+ public SharedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ Map routingDataCache) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+// TODO: Replace this Map with a real RoutingDataCache
+if (routingDataCache == null) {
+ throw new IllegalArgumentException("RoutingDataCache cannot be null!");
+}
+_routingDataCache = routingDataCache;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress = _routingDataCache.get(_zkRealmShardingKey);
+
+// Create an InnerSharedZkClient to actually serve ZK requests
+// TODO: Rename HelixZkClient in the future or remove it entirely - this
will be a backward-compatibility breaking change because HelixZkClient is being
used by Helix users.
+HelixZkClient.ZkConnectionConfig zkConnectionConfig =
+new HelixZkClient.ZkConnectionConfig(zkRealmAddress)
+.setSessionTimeout(connectionConfig.getSessionTimeout());
+_innerSharedZkClient = SharedZkClientFactory.getInstance()
Review comment:
This actually implies the ZKRealmAware SharedZKClient are mixed with the
original SharedZKCLient (now called innerSharedZKClient). We should note this
behavior.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380966871
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
##
@@ -37,79 +50,541 @@
* HelixZkClient that uses shared ZkConnection.
* A SharedZkClient won't manipulate the shared ZkConnection directly.
*/
-public class SharedZkClient extends ZkClient implements HelixZkClient {
+public class SharedZkClient implements RealmAwareZkClient {
private static Logger LOG = LoggerFactory.getLogger(SharedZkClient.class);
- /*
- * Since we cannot really disconnect the ZkConnection, we need a dummy
ZkConnection placeholder.
- * This is to ensure connection field is never null even the shared
RealmAwareZkClient instance is closed so as to avoid NPE.
- */
- private final static ZkConnection IDLE_CONNECTION = new
ZkConnection("Dummy_ZkServers");
- private final OnCloseCallback _onCloseCallback;
- private final ZkConnectionManager _connectionManager;
-
- public interface OnCloseCallback {
-/**
- * Triggered after the RealmAwareZkClient is closed.
- */
-void onClose();
- }
-
- /**
- * Construct a shared RealmAwareZkClient that uses a shared ZkConnection.
- *
- * @param connectionManager The manager of the shared ZkConnection.
- * @param clientConfig ZkClientConfig details to create the shared
RealmAwareZkClient.
- * @param callback Clean up logic when the shared
RealmAwareZkClient is closed.
- */
- public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig
clientConfig,
- OnCloseCallback callback) {
-super(connectionManager.getConnection(), 0,
clientConfig.getOperationRetryTimeout(),
-clientConfig.getZkSerializer(), clientConfig.getMonitorType(),
clientConfig.getMonitorKey(),
-clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
-_connectionManager = connectionManager;
-// Register to the base dedicated RealmAwareZkClient
-_connectionManager.registerWatcher(this);
-_onCloseCallback = callback;
+
+ private final HelixZkClient _innerSharedZkClient;
+ private final Map _routingDataCache; // TODO: replace with
RoutingDataCache
+ private final String _zkRealmShardingKey;
+
+ public SharedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ Map routingDataCache) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+// TODO: Replace this Map with a real RoutingDataCache
+if (routingDataCache == null) {
+ throw new IllegalArgumentException("RoutingDataCache cannot be null!");
+}
+_routingDataCache = routingDataCache;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress = _routingDataCache.get(_zkRealmShardingKey);
+
+// Create an InnerSharedZkClient to actually serve ZK requests
+// TODO: Rename HelixZkClient in the future or remove it entirely - this
will be a backward-compatibility breaking change because HelixZkClient is being
used by Helix users.
+HelixZkClient.ZkConnectionConfig zkConnectionConfig =
+new HelixZkClient.ZkConnectionConfig(zkRealmAddress)
+.setSessionTimeout(connectionConfig.getSessionTimeout());
+_innerSharedZkClient = SharedZkClientFactory.getInstance()
+.buildZkClient(zkConnectionConfig, (HelixZkClient.ZkClientConfig)
clientConfig);
}
@Override
- public void close() {
-super.close();
-if (isClosed()) {
- // Note that if register is not done while constructing, these private
fields may not be init yet.
- if (_connectionManager != null) {
-_connectionManager.unregisterWatcher(this);
+ public List subscribeChildChanges(String path, IZkChildListener
listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+return _innerSharedZkClient.subscribeChildChanges(path, listener);
+ }
+
+ @Override
+ public void unsubscribeChildChanges(String path, IZkChildListener listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+_innerSharedZkClient.unsubscribeChildChanges(path, listener);
+ }
+
+ @Override
+ public void subscribeDataChanges(String path, IZkDataListener
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380966871
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
##
@@ -37,79 +50,541 @@
* HelixZkClient that uses shared ZkConnection.
* A SharedZkClient won't manipulate the shared ZkConnection directly.
*/
-public class SharedZkClient extends ZkClient implements HelixZkClient {
+public class SharedZkClient implements RealmAwareZkClient {
private static Logger LOG = LoggerFactory.getLogger(SharedZkClient.class);
- /*
- * Since we cannot really disconnect the ZkConnection, we need a dummy
ZkConnection placeholder.
- * This is to ensure connection field is never null even the shared
RealmAwareZkClient instance is closed so as to avoid NPE.
- */
- private final static ZkConnection IDLE_CONNECTION = new
ZkConnection("Dummy_ZkServers");
- private final OnCloseCallback _onCloseCallback;
- private final ZkConnectionManager _connectionManager;
-
- public interface OnCloseCallback {
-/**
- * Triggered after the RealmAwareZkClient is closed.
- */
-void onClose();
- }
-
- /**
- * Construct a shared RealmAwareZkClient that uses a shared ZkConnection.
- *
- * @param connectionManager The manager of the shared ZkConnection.
- * @param clientConfig ZkClientConfig details to create the shared
RealmAwareZkClient.
- * @param callback Clean up logic when the shared
RealmAwareZkClient is closed.
- */
- public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig
clientConfig,
- OnCloseCallback callback) {
-super(connectionManager.getConnection(), 0,
clientConfig.getOperationRetryTimeout(),
-clientConfig.getZkSerializer(), clientConfig.getMonitorType(),
clientConfig.getMonitorKey(),
-clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
-_connectionManager = connectionManager;
-// Register to the base dedicated RealmAwareZkClient
-_connectionManager.registerWatcher(this);
-_onCloseCallback = callback;
+
+ private final HelixZkClient _innerSharedZkClient;
+ private final Map _routingDataCache; // TODO: replace with
RoutingDataCache
+ private final String _zkRealmShardingKey;
+
+ public SharedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ Map routingDataCache) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+// TODO: Replace this Map with a real RoutingDataCache
+if (routingDataCache == null) {
+ throw new IllegalArgumentException("RoutingDataCache cannot be null!");
+}
+_routingDataCache = routingDataCache;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress = _routingDataCache.get(_zkRealmShardingKey);
+
+// Create an InnerSharedZkClient to actually serve ZK requests
+// TODO: Rename HelixZkClient in the future or remove it entirely - this
will be a backward-compatibility breaking change because HelixZkClient is being
used by Helix users.
+HelixZkClient.ZkConnectionConfig zkConnectionConfig =
+new HelixZkClient.ZkConnectionConfig(zkRealmAddress)
+.setSessionTimeout(connectionConfig.getSessionTimeout());
+_innerSharedZkClient = SharedZkClientFactory.getInstance()
+.buildZkClient(zkConnectionConfig, (HelixZkClient.ZkClientConfig)
clientConfig);
}
@Override
- public void close() {
-super.close();
-if (isClosed()) {
- // Note that if register is not done while constructing, these private
fields may not be init yet.
- if (_connectionManager != null) {
-_connectionManager.unregisterWatcher(this);
+ public List subscribeChildChanges(String path, IZkChildListener
listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+return _innerSharedZkClient.subscribeChildChanges(path, listener);
+ }
+
+ @Override
+ public void unsubscribeChildChanges(String path, IZkChildListener listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+_innerSharedZkClient.unsubscribeChildChanges(path, listener);
+ }
+
+ @Override
+ public void subscribeDataChanges(String path, IZkDataListener
[GitHub] [helix] pkuwm commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
pkuwm commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380968738
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
##
@@ -0,0 +1,592 @@
+package org.apache.helix.zookeeper.impl.client;
+
+/*
+ * 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.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.impl.factory.MetadataStoreRoutingData;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.apache.helix.zookeeper.zkclient.IZkChildListener;
+import org.apache.helix.zookeeper.zkclient.IZkConnection;
+import org.apache.helix.zookeeper.zkclient.IZkDataListener;
+import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks;
+import org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * NOTE: DO NOT USE THIS CLASS DIRECTLY. Use DedicatedZkClientFactory to
create instances of DedicatedZkClient.
+ *
+ * An implementation of the RealmAwareZkClient interface.
+ * Supports CRUD, data change subscription, and ephemeral mode operations.
+ */
+public class DedicatedZkClient implements RealmAwareZkClient {
+ private static Logger LOG = LoggerFactory.getLogger(DedicatedZkClient.class);
+
+ private final ZkClient _rawZkClient;
+ private final MetadataStoreRoutingData _metadataStoreRoutingData;
+ private final String _zkRealmShardingKey;
+ private final String _zkRealmAddress;
+
+ public DedicatedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ MetadataStoreRoutingData metadataStoreRoutingData) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+if (metadataStoreRoutingData == null) {
+ throw new IllegalArgumentException("MetadataStoreRoutingData cannot be
null!");
+}
+_metadataStoreRoutingData = metadataStoreRoutingData;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress =
_metadataStoreRoutingData.getMetadataStoreRealm(_zkRealmShardingKey);
+if (zkRealmAddress == null || zkRealmAddress.isEmpty()) {
+ throw new IllegalArgumentException(
+ "ZK realm address for the given ZK realm sharding key is invalid! ZK
realm address: "
+ + zkRealmAddress + " ZK realm sharding key: " +
_zkRealmShardingKey);
+}
+_zkRealmAddress = zkRealmAddress;
+
+// Create a ZK connection
+IZkConnection zkConnection =
+new ZkConnection(zkRealmAddress, connectionConfig.getSessionTimeout());
+
+// Create a ZkClient
+_rawZkClient = new ZkClient(zkConnection, (int)
clientConfig.getConnectInitTimeout(),
+clientConfig.getOperationRetryTimeout(),
clientConfig.getZkSerializer(),
+clientConfig.getMonitorType(), clientConfig.getMonitorKey(),
+clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
+ }
+
+ @Override
+ public List subscribeChildChanges(String path, IZkChildListener
listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+return
[GitHub] [helix] pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
URL: https://github.com/apache/helix/pull/775#discussion_r381014954
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/util/HttpRoutingDataReader.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.zookeeper.util;
+
+/*
+ * 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.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.map.ObjectMapper;
+
+
+public class HttpRoutingDataReader {
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ /**
+ * This class is a Singleton.
+ */
+ private HttpRoutingDataReader() {
+ }
+
+ /**
+ * Fetches routing data from the data source via HTTP.
+ * @return a mapping from "metadata store realm addresses" to lists of
"metadata store sharding keys", where the sharding keys in a value list all
route to the realm address in the key disallows a meaningful mapping to be
returned
Review comment:
Nit, wrap long line?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
URL: https://github.com/apache/helix/pull/775#discussion_r381014832
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/util/HttpRoutingDataReader.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.zookeeper.util;
+
+/*
+ * 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.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.map.ObjectMapper;
+
+
+public class HttpRoutingDataReader {
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ /**
+ * This class is a Singleton.
+ */
+ private HttpRoutingDataReader() {
+ }
+
+ /**
+ * Fetches routing data from the data source via HTTP.
+ * @return a mapping from "metadata store realm addresses" to lists of
"metadata store sharding keys", where the sharding keys in a value list all
route to the realm address in the key disallows a meaningful mapping to be
returned
+ */
+ public static Map> getRoutingData(String msdsEndpoint) {
+
+Map> rawRoutingData = new HashMap<>();
+
+HttpGet requestAllRealmNames =
+new HttpGet(msdsEndpoint); //TODO: construct an endpoint once REST
endpoint is finalized
+try (CloseableHttpClient httpClient = HttpClients.createDefault();
+CloseableHttpResponse response =
httpClient.execute(requestAllRealmNames)) {
Review comment:
1. Since this is a blocking IO, my thought is it may add more time for
zkclient init/creation. Would it help if the request is async while creating a
new zkclient?
2. Do we need to consider timeout for this request?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
URL: https://github.com/apache/helix/pull/775#discussion_r381008006
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/util/HttpRoutingDataReader.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.zookeeper.util;
+
+/*
+ * 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.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.map.ObjectMapper;
+
+
+public class HttpRoutingDataReader {
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ /**
+ * This class is a Singleton.
+ */
+ private HttpRoutingDataReader() {
+ }
+
+ /**
+ * Fetches routing data from the data source via HTTP.
+ * @return a mapping from "metadata store realm addresses" to lists of
"metadata store sharding keys", where the sharding keys in a value list all
route to the realm address in the key disallows a meaningful mapping to be
returned
Review comment:
Nit, wrap long line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r381032011
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,106 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo {
- //TODO: add specific setter and getter for any field that is determined to
be universal for all implementations of HelixLock
+ // Default values for each attribute if there are no current values set by
user
+ public static final String DEFAULT_OWNER_TEXT = "";
+ public static final String DEFAULT_MESSAGE_TEXT = "";
+ public static final long DEFAULT_TIMEOUT_LONG = -1L;
+
+ // default lock info represents the status of a unlocked lock
+ public static final LockInfo defaultLockInfo =
+ new LockInfo(DEFAULT_OWNER_TEXT, DEFAULT_MESSAGE_TEXT,
DEFAULT_TIMEOUT_LONG);
+
+ private ZNRecord record;
+
+ /**
+ * The keys to lock information
+ */
+ public enum LockInfoAttribute {
+OWNER, MESSAGE, TIMEOUT
+ }
+
+ /**
+ * Initialize a default LockInfo instance
+ */
+ private LockInfo() {
+record = new ZNRecord("LOCK");
Review comment:
Make "LOCK" to be a constant instead of hard coding 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r381032659
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,106 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo {
- //TODO: add specific setter and getter for any field that is determined to
be universal for all implementations of HelixLock
+ // Default values for each attribute if there are no current values set by
user
+ public static final String DEFAULT_OWNER_TEXT = "";
+ public static final String DEFAULT_MESSAGE_TEXT = "";
+ public static final long DEFAULT_TIMEOUT_LONG = -1L;
+
+ // default lock info represents the status of a unlocked lock
+ public static final LockInfo defaultLockInfo =
+ new LockInfo(DEFAULT_OWNER_TEXT, DEFAULT_MESSAGE_TEXT,
DEFAULT_TIMEOUT_LONG);
+
+ private ZNRecord record;
Review comment:
naming convention: _record;
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r381031816
##
File path: helix-lock/src/main/java/org/apache/helix/lock/HelixLock.java
##
@@ -22,7 +22,7 @@
/**
* Generic interface for Helix distributed lock
*/
-public interface HelixLock {
+public interface HelixLock {
Review comment:
Let's have the interface as DistributedLock or something. Name it HelixLock
still seemed to be sticking to Helix.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r381031871
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,106 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo {
- //TODO: add specific setter and getter for any field that is determined to
be universal for all implementations of HelixLock
+ // Default values for each attribute if there are no current values set by
user
+ public static final String DEFAULT_OWNER_TEXT = "";
+ public static final String DEFAULT_MESSAGE_TEXT = "";
+ public static final long DEFAULT_TIMEOUT_LONG = -1L;
+
+ // default lock info represents the status of a unlocked lock
+ public static final LockInfo defaultLockInfo =
+ new LockInfo(DEFAULT_OWNER_TEXT, DEFAULT_MESSAGE_TEXT,
DEFAULT_TIMEOUT_LONG);
+
+ private ZNRecord record;
+
+ /**
+ * The keys to lock information
+ */
+ public enum LockInfoAttribute {
+OWNER, MESSAGE, TIMEOUT
Review comment:
Make one element per line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #718: Implement Helix nonblocking lock
mgao0 commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380962857
##
File path:
helix-lock/src/main/java/org/apache/helix/lock/helix/ZKHelixNonblockingLock.java
##
@@ -0,0 +1,145 @@
+/*
+ * 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.
+ */
+
+package org.apache.helix.lock.helix;
+
+import java.util.Date;
+
+import org.I0Itec.zkclient.DataUpdater;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.lock.HelixLock;
+import org.apache.helix.lock.LockInfo;
+import org.apache.helix.lock.LockScope;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.log4j.Logger;
+
+
+/**
+ * Helix nonblocking lock implementation based on Zookeeper
+ */
+public class ZKHelixNonblockingLock implements HelixLock {
+
+ private static final Logger LOG =
Logger.getLogger(ZKHelixNonblockingLock.class);
+
+ private final String _lockPath;
+ private final String _userId;
+ private final long _timeout;
+ private final String _lockMsg;
+ private final BaseDataAccessor _baseDataAccessor;
+
+ /**
+ * Initialize the lock with user provided information, e.g.,cluster, scope,
etc.
+ * @param scope the scope to lock
+ * @param zkAddress the zk address the cluster connects to
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ public ZKHelixNonblockingLock(LockScope scope, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+this(scope.getPath(), zkAddress, timeout, lockMsg, userId);
+ }
+
+ /**
+ * Initialize the lock with user provided information, e.g., lock path under
zookeeper, etc.
+ * @param lockPath the path of the lock under Zookeeper
+ * @param zkAddress the zk address of the cluster
+ * @param timeout the timeout period of the lcok
+ * @param lockMsg the reason for having this lock
+ * @param userId a universal unique userId for lock owner identity
+ */
+ private ZKHelixNonblockingLock(String lockPath, String zkAddress, Long
timeout, String lockMsg,
+ String userId) {
+_lockPath = lockPath;
+if (timeout < 0) {
+ throw new IllegalArgumentException("The expiration time cannot be
negative.");
+}
+_timeout = timeout;
+_lockMsg = lockMsg;
+_userId = userId;
+_baseDataAccessor = new ZkBaseDataAccessor<>(zkAddress);
+ }
+
+ @Override
+ public boolean acquireLock() {
+
+// Set lock information fields
+long deadline;
+// Prevent value overflow
+if (_timeout > Long.MAX_VALUE - System.currentTimeMillis()) {
+ deadline = Long.MAX_VALUE;
+} else {
+ deadline = System.currentTimeMillis() + _timeout;
+}
+LockUpdater updater = new LockUpdater(new LockInfo(_userId, _lockMsg,
deadline));
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
+ }
+
+ @Override
+ public boolean releaseLock() {
+// Initialize the lock updater with a default lock info represents the
state of a unlocked lock
+LockUpdater updater = new LockUpdater(LockInfo.defaultLockInfo);
+return _baseDataAccessor.update(_lockPath, updater,
AccessOption.PERSISTENT);
Review comment:
Directly deleting it can cause race conditions. For example, when we call
delete to delete lock node A, but during the process someone else acquired the
lock or the current lock owner updated the lock, so currently it's lock node B,
as a result the delete call deletes lock node B instead of lock node A, and
both the caller and the lock owner wouldn't be aware of this mistake. The
update method checks the version of the znode, so it can prevent this situation.
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:
us...@infra.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380989661
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##
@@ -82,13 +94,8 @@ public HelixZkClient
buildZkClient(HelixZkClient.ZkConnectionConfig connectionCo
throw new ZkClientException("Failed to create a connection manager in
the pool to share.");
}
LOG.info("Sharing ZkConnection {} to a new SharedZkClient.",
connectionConfig.toString());
- return new SharedZkClient(zkConnectionManager, clientConfig,
- new SharedZkClient.OnCloseCallback() {
-@Override
-public void onClose() {
- cleanupConnectionManager(zkConnectionManager);
-}
- });
+ return new InnerSharedZkClient(zkConnectionManager, clientConfig,
Review comment:
nit: the intention is to keeper old sharedZkClient the same, but use new
name innerSharedZkClient. let us don't use the new Java feature to rewrite the
same logic. Just leave it as what it were. It would be easier to people to look
at the history to understand the intention.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
NealSun96 commented on a change in pull request #761: Add REST read endpoints
to helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r380992746
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +135,74 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
+ *
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
}
} catch (NoSuchElementException ex) {
return notFound(ex.getMessage());
}
+ }
-responseMap.put(MetadataStoreRoutingConstants.SHARDING_KEYS, shardingKeys);
-
-return JSONRepresentation(responseMap);
+ /**
+ * Gets all path-based sharding keys for a queried realm at endpoint:
+ * "GET /metadata-store-realms/{realm}/sharding-keys". This endpoint is
equivalent to
+ * the endpoint: "GET /sharding-keys?realm={realm}".
+ *
+ * @param realm Queried metadata store realm to get sharding keys.
+ * @return All path-based sharding keys in the queried realm.
+ */
+ @GET
+ @Path("/metadata-store-realms/{realm}/sharding-keys")
+ public Response getRealmShardingKeys(@PathParam("realm") String realm) {
+try {
+ return getAllShardingKeysInRealm(realm);
+} catch (NoSuchElementException ex) {
+ return notFound(ex.getMessage());
Review comment:
Given my understanding to the status code, `badRequest` is more suitable
than `notFound` in this situation. What `notFound` entails is that this url is
not matched to anything, however in this situation, the url is correctly
connecting to the endpoint. Returning `notFound` might cause confusion.
I understand that
[GitHub] [helix] pkuwm opened a new issue #779: Implement FederatedZkClient to Support Multi-Realm CRUD and Callback
pkuwm opened a new issue #779: Implement FederatedZkClient to Support Multi-Realm CRUD and Callback URL: https://github.com/apache/helix/issues/779 FederatedZkClient (One FederatedZkClient, multiple Zookeeper sessions inside) - Some Helix Java APIs such as HelixAdmin can only perform CRUD operations. - In order to enable such Java APIs to operate against multiple Zookeeper realms, we will create FederatedZkClient, which is a new version of ZkClient that could replace the existing HelixZkClient used by CRUD-only Java APIs. - FederatedZkClient will internally maintain multiple ZooKeeper sessions connecting to different ZooKeeper realms on an as-needed basis. - By creating and maintaining multiple ZooKeeper sessions, FederatedZkClient will route incoming ZooKeeper requests to the appropriate ZooKeeper sessions based on the ZK path sharding key. FederatedZkClient (implements RealmAwareZkClient) - An implementation of RealmAwareZkClient that connects to multiple ZK realms. - Supports CRUD + change subscription. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #776: add CustomizedStateAggregation config
zhangmeng916 commented on a change in pull request #776: add
CustomizedStateAggregation config
URL: https://github.com/apache/helix/pull/776#discussion_r380982556
##
File path:
helix-core/src/main/java/org/apache/helix/model/CustomizedStateAggregationConfig.java
##
@@ -0,0 +1,144 @@
+package org.apache.helix.model;
+
+/*
+ * 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.ArrayList;
+import java.util.List;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
+
+/**
+ * CustomizedStateAggregation configurations
+ */
+public class CustomizedStateAggregationConfig extends HelixProperty {
+ /**
+ * Indicate which customized states will be aggregated.
+ * NOTE: Do NOT use this field name directly, use its corresponding
getter/setter in the
+ * CustomizedStateAggregationConfig.
+ */
+ public enum CustomizedStateAggregationProperty {
+AGGREGATION_ENABLED_STATES,
Review comment:
Discussed offline. We'll use type to refer each individual customized state.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient and update SharedZkClientFactory
kaisun2000 commented on a change in pull request #770: WIP: Add SharedZkClient
and update SharedZkClientFactory
URL: https://github.com/apache/helix/pull/770#discussion_r380961009
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
##
@@ -37,79 +50,541 @@
* HelixZkClient that uses shared ZkConnection.
* A SharedZkClient won't manipulate the shared ZkConnection directly.
*/
-public class SharedZkClient extends ZkClient implements HelixZkClient {
+public class SharedZkClient implements RealmAwareZkClient {
private static Logger LOG = LoggerFactory.getLogger(SharedZkClient.class);
- /*
- * Since we cannot really disconnect the ZkConnection, we need a dummy
ZkConnection placeholder.
- * This is to ensure connection field is never null even the shared
RealmAwareZkClient instance is closed so as to avoid NPE.
- */
- private final static ZkConnection IDLE_CONNECTION = new
ZkConnection("Dummy_ZkServers");
- private final OnCloseCallback _onCloseCallback;
- private final ZkConnectionManager _connectionManager;
-
- public interface OnCloseCallback {
-/**
- * Triggered after the RealmAwareZkClient is closed.
- */
-void onClose();
- }
-
- /**
- * Construct a shared RealmAwareZkClient that uses a shared ZkConnection.
- *
- * @param connectionManager The manager of the shared ZkConnection.
- * @param clientConfig ZkClientConfig details to create the shared
RealmAwareZkClient.
- * @param callback Clean up logic when the shared
RealmAwareZkClient is closed.
- */
- public SharedZkClient(ZkConnectionManager connectionManager, ZkClientConfig
clientConfig,
- OnCloseCallback callback) {
-super(connectionManager.getConnection(), 0,
clientConfig.getOperationRetryTimeout(),
-clientConfig.getZkSerializer(), clientConfig.getMonitorType(),
clientConfig.getMonitorKey(),
-clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
-_connectionManager = connectionManager;
-// Register to the base dedicated RealmAwareZkClient
-_connectionManager.registerWatcher(this);
-_onCloseCallback = callback;
+
+ private final HelixZkClient _innerSharedZkClient;
+ private final Map _routingDataCache; // TODO: replace with
RoutingDataCache
+ private final String _zkRealmShardingKey;
+
+ public SharedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ Map routingDataCache) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+// TODO: Replace this Map with a real RoutingDataCache
+if (routingDataCache == null) {
+ throw new IllegalArgumentException("RoutingDataCache cannot be null!");
+}
+_routingDataCache = routingDataCache;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress = _routingDataCache.get(_zkRealmShardingKey);
+
+// Create an InnerSharedZkClient to actually serve ZK requests
+// TODO: Rename HelixZkClient in the future or remove it entirely - this
will be a backward-compatibility breaking change because HelixZkClient is being
used by Helix users.
+HelixZkClient.ZkConnectionConfig zkConnectionConfig =
+new HelixZkClient.ZkConnectionConfig(zkRealmAddress)
+.setSessionTimeout(connectionConfig.getSessionTimeout());
+_innerSharedZkClient = SharedZkClientFactory.getInstance()
Review comment:
This actually implies the ZKRealmAware SharedZKClient are mixed with the
original SharedZKCLient (now called innerSharedZKClient). We should note this
behavior.
Put it another way, underlying the same ZkConnection manager, there can be
innersharedZkClient which do not check path validity and new SharedZkClient
which does check path validity.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 opened a new issue #778: Add leader request forwarding for ZkRoutingDataWriter
NealSun96 opened a new issue #778: Add leader request forwarding for ZkRoutingDataWriter URL: https://github.com/apache/helix/issues/778 Due to the potential concurrency issues raised by multiple writes to ZooKeeper, we have decided on the design of having one single writer for `ZkRoutingDataWriter`. More specifically, by utilizing `ZkDistributedLeaderElection`, one instance of the writer will be elected as the leader, and all other instances will forward write requests to the leader. We need to add logic in `ZkRoutingDataWriter` to support 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
pkuwm commented on a change in pull request #775: WIP: Add HttpRoutingDataReader
URL: https://github.com/apache/helix/pull/775#discussion_r381014954
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/util/HttpRoutingDataReader.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.zookeeper.util;
+
+/*
+ * 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.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.map.ObjectMapper;
+
+
+public class HttpRoutingDataReader {
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ /**
+ * This class is a Singleton.
+ */
+ private HttpRoutingDataReader() {
+ }
+
+ /**
+ * Fetches routing data from the data source via HTTP.
+ * @return a mapping from "metadata store realm addresses" to lists of
"metadata store sharding keys", where the sharding keys in a value list all
route to the realm address in the key disallows a meaningful mapping to be
returned
Review comment:
Nit, wrap long line?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
pkuwm commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380968738
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
##
@@ -0,0 +1,592 @@
+package org.apache.helix.zookeeper.impl.client;
+
+/*
+ * 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.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.impl.factory.MetadataStoreRoutingData;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.apache.helix.zookeeper.zkclient.IZkChildListener;
+import org.apache.helix.zookeeper.zkclient.IZkConnection;
+import org.apache.helix.zookeeper.zkclient.IZkDataListener;
+import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks;
+import org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * NOTE: DO NOT USE THIS CLASS DIRECTLY. Use DedicatedZkClientFactory to
create instances of DedicatedZkClient.
+ *
+ * An implementation of the RealmAwareZkClient interface.
+ * Supports CRUD, data change subscription, and ephemeral mode operations.
+ */
+public class DedicatedZkClient implements RealmAwareZkClient {
+ private static Logger LOG = LoggerFactory.getLogger(DedicatedZkClient.class);
+
+ private final ZkClient _rawZkClient;
+ private final MetadataStoreRoutingData _metadataStoreRoutingData;
+ private final String _zkRealmShardingKey;
+ private final String _zkRealmAddress;
+
+ public DedicatedZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig
connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig,
+ MetadataStoreRoutingData metadataStoreRoutingData) {
+
+if (connectionConfig == null) {
+ throw new IllegalArgumentException("RealmAwareZkConnectionConfig cannot
be null!");
+}
+_zkRealmShardingKey = connectionConfig.getZkRealmShardingKey();
+
+if (metadataStoreRoutingData == null) {
+ throw new IllegalArgumentException("MetadataStoreRoutingData cannot be
null!");
+}
+_metadataStoreRoutingData = metadataStoreRoutingData;
+
+// Get the ZkRealm address based on the ZK path sharding key
+String zkRealmAddress =
_metadataStoreRoutingData.getMetadataStoreRealm(_zkRealmShardingKey);
+if (zkRealmAddress == null || zkRealmAddress.isEmpty()) {
+ throw new IllegalArgumentException(
+ "ZK realm address for the given ZK realm sharding key is invalid! ZK
realm address: "
+ + zkRealmAddress + " ZK realm sharding key: " +
_zkRealmShardingKey);
+}
+_zkRealmAddress = zkRealmAddress;
+
+// Create a ZK connection
+IZkConnection zkConnection =
+new ZkConnection(zkRealmAddress, connectionConfig.getSessionTimeout());
+
+// Create a ZkClient
+_rawZkClient = new ZkClient(zkConnection, (int)
clientConfig.getConnectInitTimeout(),
+clientConfig.getOperationRetryTimeout(),
clientConfig.getZkSerializer(),
+clientConfig.getMonitorType(), clientConfig.getMonitorKey(),
+clientConfig.getMonitorInstanceName(),
clientConfig.isMonitorRootPathOnly());
+ }
+
+ @Override
+ public List subscribeChildChanges(String path, IZkChildListener
listener) {
+if (!checkIfPathBelongsToZkRealm(path)) {
+ throw new IllegalArgumentException(
+ "The given path does not map to the ZK realm for this
DedicatedZkClient! Path: " + path
+ + " ZK realm sharding key: " + _zkRealmShardingKey);
+}
+return
[GitHub] [helix] pkuwm commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory
pkuwm commented on a change in pull request #765: Add DedicatedZkClient and
update DedicatedZkClientFactory
URL: https://github.com/apache/helix/pull/765#discussion_r380963631
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
##
@@ -0,0 +1,592 @@
+package org.apache.helix.zookeeper.impl.client;
+
+/*
+ * 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.List;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.impl.factory.MetadataStoreRoutingData;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.apache.helix.zookeeper.zkclient.IZkChildListener;
+import org.apache.helix.zookeeper.zkclient.IZkConnection;
+import org.apache.helix.zookeeper.zkclient.IZkDataListener;
+import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks;
+import org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * NOTE: DO NOT USE THIS CLASS DIRECTLY. Use DedicatedZkClientFactory to
create instances of DedicatedZkClient.
+ *
+ * An implementation of the RealmAwareZkClient interface.
+ * Supports CRUD, data change subscription, and ephemeral mode operations.
+ */
+public class DedicatedZkClient implements RealmAwareZkClient {
Review comment:
If we don't want users to directly use this class, shall we consider
refactoring the code structure so we can make this class `protected`? I think
that may be better than just commenting not to use it directly.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r381000691
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +135,74 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
+ *
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
}
} catch (NoSuchElementException ex) {
return notFound(ex.getMessage());
}
+ }
-responseMap.put(MetadataStoreRoutingConstants.SHARDING_KEYS, shardingKeys);
-
-return JSONRepresentation(responseMap);
+ /**
+ * Gets all path-based sharding keys for a queried realm at endpoint:
+ * "GET /metadata-store-realms/{realm}/sharding-keys". This endpoint is
equivalent to
+ * the endpoint: "GET /sharding-keys?realm={realm}".
+ *
+ * @param realm Queried metadata store realm to get sharding keys.
+ * @return All path-based sharding keys in the queried realm.
+ */
+ @GET
+ @Path("/metadata-store-realms/{realm}/sharding-keys")
+ public Response getRealmShardingKeys(@PathParam("realm") String realm) {
+try {
+ return getAllShardingKeysInRealm(realm);
+} catch (NoSuchElementException ex) {
+ return notFound(ex.getMessage());
Review comment:
@NealSun96 Thanks for reviewing.
As you referred in the link, `400 Bad Request`: The request could not be
understood by the server due to **malformed syntax**. If you request a POST/PUT
with json payload which has syntax error, I believe `Bad Request` is more
suitable.
`404 Not Found` means the endpoint is correct but
[GitHub] [helix] dasahcc commented on a change in pull request #718: Implement Helix nonblocking lock
dasahcc commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r381031248
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,109 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo extends HelixProperty {
Review comment:
Sounds good.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #718: Implement Helix nonblocking lock
mgao0 commented on a change in pull request #718: Implement Helix nonblocking
lock
URL: https://github.com/apache/helix/pull/718#discussion_r380964332
##
File path: helix-lock/src/main/java/org/apache/helix/lock/LockInfo.java
##
@@ -19,28 +19,109 @@
package org.apache.helix.lock;
-import java.util.Map;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
/**
- * Generic interface for a map contains the Helix lock information
- * @param The type of the LockInfo value
+ * Structure represents a lock node information, implemented using ZNRecord
*/
-public interface LockInfo {
+public class LockInfo extends HelixProperty {
Review comment:
In this case I can remove the "extends HelixProperty" to make it independent
from Helix. The LockInfo is barely a ZNRecord, and currently I'm not using any
properties and methods exclusively provided by HelixProperty.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
NealSun96 commented on a change in pull request #761: Add REST read endpoints
to helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r380994410
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
Review comment:
There is a good way to simplify the logic here utilizing the default values
as @dasahcc has mentioned. If the prefix is not provided, consider assigning
"/" to the prefix variable and use the same logic as the situation where prefix
exists.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
NealSun96 commented on a change in pull request #761: Add REST read endpoints
to helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r380997605
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -110,7 +110,33 @@ public void beforeClass() throws Exception {
_metadataStoreDirectory = new ZkMetadataStoreDirectory(routingZkAddrMap);
}
+ /*
+ * Tests REST endpoint: "GET
/namespaces/{namespace}/metadata-store-namespaces"
+ */
@Test
+ public void testGetAllNamespaces() throws IOException {
+String responseBody = get(TEST_NAMESPACE_URI_PREFIX +
"/metadata-store-namespaces", null,
+Response.Status.OK.getStatusCode(), true);
+
+// It is safe to cast the object and suppress warnings.
+@SuppressWarnings("unchecked")
+Map> queriedNamespacesMap =
+OBJECT_MAPPER.readValue(responseBody, Map.class);
+
+Assert.assertTrue(
+
queriedNamespacesMap.containsKey(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES));
Review comment:
When verifying a map, I'd make sure that "it contains the right elements and
**only** the right elements". Therefore I usually perform a check on map length
- without it if the map returns something extra and unexpected, the test cases
would still incorrectly pass.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r381044344
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -110,7 +110,33 @@ public void beforeClass() throws Exception {
_metadataStoreDirectory = new ZkMetadataStoreDirectory(routingZkAddrMap);
}
+ /*
+ * Tests REST endpoint: "GET
/namespaces/{namespace}/metadata-store-namespaces"
+ */
@Test
+ public void testGetAllNamespaces() throws IOException {
+String responseBody = get(TEST_NAMESPACE_URI_PREFIX +
"/metadata-store-namespaces", null,
+Response.Status.OK.getStatusCode(), true);
+
+// It is safe to cast the object and suppress warnings.
+@SuppressWarnings("unchecked")
+Map> queriedNamespacesMap =
+OBJECT_MAPPER.readValue(responseBody, Map.class);
+
+Assert.assertTrue(
+
queriedNamespacesMap.containsKey(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES));
Review comment:
Sure. I was checking the response has this field. It is definitely more
accurate to check that the response has only expected fields.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r381053697
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
Review comment:
> There is a good way to simplify the logic here utilizing the default
values as @dasahcc has mentioned. If the prefix is not provided, consider
assigning "/" to the prefix variable and use the same logic as the situation
where prefix exists.
@NealSun96 I don't think adding "/" to the prefix var is a better idea
because:
- The Json responses are different for each case here. If you assign "/" to
a method, the response could have `{ "prefix": "/" }`
- I've tried to find a good default value for this prefix. If I assign "/"
as a default value, I've no idea how to differentiate if prefix is not provide
or it is "/".
- The code logic to generate response map is different.
If I don't understand your idea, feel free to clarify it. Thanks for the tip.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To
[GitHub] [helix] narendly merged pull request #759: Add validation logic to MSD write operations
narendly merged pull request #759: Add validation logic to MSD write operations URL: https://github.com/apache/helix/pull/759 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] zhangmeng916 opened a new pull request #776: add CustomizedStateAggregation config
zhangmeng916 opened a new pull request #776: add CustomizedStateAggregation
config
URL: https://github.com/apache/helix/pull/776
### Issues
- [X] My PR addresses the following Helix issues and references them in the
PR description:
(#728 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI
changes:
Helix will introduce the concept of customized state, which allows customer
to define the states they are interested. Helix will also help aggregate these
states for customers' easier usage. This PR implements a new cluster level
config called customized state aggregation config. If customer would like Helix
to aggregate their customized states, they need to add the specific customized
state in this config.
### Tests
- [X] The following tests are written for this issue:
TestCustomizedStateAggregationConfig
- [ ] 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, and I have squashed multiple commits if they address the
same issue. 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
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on issue #759: Add validation logic to MSD write operations
NealSun96 commented on issue #759: Add validation logic to MSD write operations URL: https://github.com/apache/helix/pull/759#issuecomment-587580298 This PR is ready to be merged, approved by @narendly Final commit message: ## Add validation logic to MSD write operations ## This PR add routing data validation logic MSD writing operations to ensure the writes leave routing data in a valid state. TrieRoutingData logic is modified to reduce duplicate 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on issue #772: Unexpected randomness in the WAGED rebalancer partition assignment
NealSun96 commented on issue #772: Unexpected randomness in the WAGED rebalancer partition assignment URL: https://github.com/apache/helix/issues/772#issuecomment-587599600 Please assign this issue to me, @jiajunwang , thank you! 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
