[GitHub] [helix] pkuwm closed issue #539: Add instance utilization metrics
pkuwm closed issue #539: Add instance utilization metrics URL: https://github.com/apache/helix/issues/539 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 closed issue #544: Any exception from metric collector in waged rebalancer may break rebalancer
pkuwm closed issue #544: Any exception from metric collector in waged rebalancer may break rebalancer URL: https://github.com/apache/helix/issues/544 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] codeon opened a new issue #683: Inconsistency in the nodes tracked
codeon opened a new issue #683: Inconsistency in the nodes tracked URL: https://github.com/apache/helix/issues/683 Hi, We are seeing this particular issue, where our nodes are not able to start the participant process and the de-registration process fails with the following exception: ``` 2019-10-03 05:31:28 [core-thread-12] ERROR c.uber.streamgate.helix.Participant - Exception while unregistering helix participant org.apache.helix.HelixException: Node dca1-prod05_streamgate_shadow_0 does not exist in config for cluster StreamgateClusterV1-DCA1-Shadow at org.apache.helix.manager.zk.ZKHelixAdmin.dropInstance(ZKHelixAdmin.java:129) at c.u.s.helix.Participant.unregister(Participant.java:134) at c.u.s.helix.Participant.registerInstance(Participant.java:109) at c.u.s.helix.Participant.run(Participant.java:61) at c.u.s.http.endpoints.HelixRegister.lambda$doHandle$0(HelixRegister.java:57) at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163) at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:404) at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:309) at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:884) at java.lang.Thread.run(Thread.java:748) ``` However, when the node tries to register itself, it gets an exception saying that the node is already registered. From our understanding, it happens when under some extreme circumstances (like a flappy node restarting quickly), instance information goes away from `//CONFIGS/PARTICIPANT` path but gets stuck in `//INSTANCES/` path. Then all helix commands, register/unregister/delete/disable fail. In order to fix it, we remove the node from `//INSTANCES/` path manually, and restart controller processes and the participant nodes so they can register cleanly again. We wanted to understand when can such a situation arise when the instance is cleaned up from one path but remains in another leading to inconsistency. Helix Version : 0.8.2 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 #675: Add REST API for Cluster Creation with CloudConfig
alirezazamani commented on a change in pull request #675: Add REST API for
Cluster Creation with CloudConfig
URL: https://github.com/apache/helix/pull/675#discussion_r367211070
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##
@@ -514,6 +533,119 @@ private boolean doesClusterExist(String cluster) {
return ZKUtil.isClusterSetup(cluster, zkClient);
}
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound("Cluster is not properly setup!");
+}
+
+HelixAdmin admin = getHelixAdmin();
+ZNRecord record;
+try {
+ record = toZNRecord(content);
+} catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ",
Exception: " + e);
+ return badRequest("Input is not a vaild ZNRecord!");
+}
+
+try {
+ CloudConfig cloudConfig = new CloudConfig.Builder(record).build();
+ admin.addCloudConfig(clusterId, cloudConfig);
+} catch (HelixException ex) {
+ _logger.error("Error in adding a CloudConfig to cluster: " + clusterId,
ex);
+ return badRequest(ex.getMessage());
+} catch (Exception ex) {
+ _logger.error("Cannot add CloudConfig to cluster: " + clusterId, ex);
+ return badRequest(ex.getMessage());
Review comment:
Fixed. Thanks.
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 #653: add Helix properties factory and class
zhangmeng916 commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r367144265
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -76,9 +77,16 @@
final StateMachineEngine _stateMachineEngine;
final LiveInstanceInfoProvider _liveInstanceInfoProvider;
final List _preConnectCallbacks;
+ final HelixParticipantProperty _helixParticipantProperty;
public ParticipantManager(HelixManager manager, HelixZkClient zkclient, int
sessionTimeout,
LiveInstanceInfoProvider liveInstanceInfoProvider,
List preConnectCallbacks) {
+this(manager, zkclient, sessionTimeout, liveInstanceInfoProvider,
preConnectCallbacks, null);
Review comment:
I mark it as deprecated.
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 #653: add Helix properties factory and class
zhangmeng916 commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r367143348
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,175 @@
+package org.apache.helix;
+
+/*
+ * 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.InputStream;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Hold helix cloud properties read from CloudConfig and user defined files.
Clients may override the fields from their application.
+ */
+public class HelixCloudProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixCloudProperty.class.getName());
+ private static final String AZURE_CLOUD_PROPERTY_FILE =
SystemPropertyKeys.AZURE_CLOUD_PROPERTIES;
+ private static final String CLOUD_INFO_SOURCE = "cloud_info_source";
+ private static final String CLOUD_INFO_PROCESSFOR_NAME =
"cloud_info_processor_name";
+ private static final String CLOUD_MAX_RETRY = "cloud_max_retry";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ // Denote whether the instance is considered as in a cloud environment.
+ private boolean _isCloudEnabled;
+
+ // Unique id of the cloud environment where the instance is in.
+ private String _cloudId;
+
+ // Cloud environment provider, e.g. Azure, AWS, GCP, etc.
+ private String _cloudProvider;
+
+ // The sources where the cloud instance information can be retrieved from.
+ private List _cloudInfoSources;
+
+ // The name of the function that will fetch and parse cloud instance
information.
+ private String _cloudInfoProcessorName;
+
+ // Http max retry times when querying the cloud instance information from
cloud environment.
+ private int _cloudMaxRetry;
+
+ // Http connection time when querying the cloud instance information from
cloud environment.
+ private long _cloudConnectionTimeout;
+
+ // Http request timeout when querying the cloud instance information from
cloud environment.
+ private long _cloudRequestTimeout;
+
+ // Other customized properties that may be used.
+ private Properties _customizedCloudProperties = new Properties();
+
+ /**
+ * Initialize Helix Cloud Property based on the provider
+ * @param
+ */
+ public HelixCloudProperty(CloudConfig cloudConfig) {
+setCloudEndabled(cloudConfig.isCloudEnabled());
+setCloudId(cloudConfig.getCloudID());
+setCloudProvider(cloudConfig.getCloudProvider());
+switch (CloudProvider.valueOf(cloudConfig.getCloudProvider())) {
+ case AZURE:
+Properties azureProperties = new Properties();
+try {
+ InputStream stream =
+
Thread.currentThread().getContextClassLoader().getResourceAsStream(AZURE_CLOUD_PROPERTY_FILE);
+ azureProperties.load(stream);
+} catch (Exception e) {
+ String errMsg = "failed to open Helix Azure cloud properties file: "
+ AZURE_CLOUD_PROPERTY_FILE;
+ throw new IllegalArgumentException(errMsg, e);
+}
+LOG.info("Successfully loaded Helix Azure cloud properties: " +
azureProperties);
+
setCloudInfoSources(Collections.singletonList(azureProperties.getProperty(CLOUD_INFO_SOURCE)));
+
setCloudInfoProcessorName(azureProperties.getProperty(CLOUD_INFO_PROCESSFOR_NAME));
+setCloudMaxRetry(azureProperties.getProperty(CLOUD_MAX_RETRY));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
+ case CUSTOMIZED:
+setCloudInfoSources(cloudConfig.getCloudInfoSources());
+setCloudInfoProcessorName(cloudConfig.getCloudInfoProcessorName());
+ default:
+LOG.info("Unsupported cloud provider: " +
cloudConfig.getCloudProvider());
+}
+ }
+
+ public boolean getCloudEnabled() {
+return
[GitHub] [helix] zhangmeng916 commented on a change in pull request #653: add Helix properties factory and class
zhangmeng916 commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r367144088
##
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##
@@ -0,0 +1,75 @@
+package org.apache.helix;
+
+/*
+ * 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.Properties;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Hold Helix participant properties. The participant properties further hold
Helix cloud properties and some other properties
+ * specific for the participant.
+ */
+public class HelixManagerProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixManagerProperty.class.getName());
+ private String _version;
+ private long _healthReportLatency;
+ private HelixCloudProperty _helixCloudProperty;
+
+ /**
+ * Initialize Helix manager property with default value
+ * @param properties helix manager related properties input as a map
+ * @param cloudConfig cloudConfig read from Zookeeper
+ */
+ public HelixManagerProperty(Properties properties, CloudConfig cloudConfig) {
+if (cloudConfig != null) {
+ _helixCloudProperty = new HelixCloudProperty(cloudConfig);
Review comment:
The cloudEnabled flag is actually in cloud config itself. Remember that we
talk about we do not want to touch cluster config, and we put all cloud related
configs in a separate znode.
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 #653: add Helix properties factory and class
zhangmeng916 commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r367131143
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkClusterManager.java
##
@@ -63,18 +63,11 @@ public void testController() throws Exception {
_gZkClient.deleteRecursively("/" + clusterName);
}
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+
ZKHelixManager controller =
new ZKHelixManager(clusterName, null, InstanceType.CONTROLLER,
ZK_ADDR);
-try {
- controller.connect();
- Assert.fail("Should throw HelixException if initial cluster structure is
not setup");
Review comment:
The problem with the previous test is that the call to get default Helix
manager property will fail because we have not set it up yet. The old
ZKHelixManager constructor has no dependency on cluster set up, but the new one
needs to get the cloud config from 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 #675: Add REST API for Cluster Creation with CloudConfig
dasahcc commented on a change in pull request #675: Add REST API for Cluster
Creation with CloudConfig
URL: https://github.com/apache/helix/pull/675#discussion_r367131650
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##
@@ -514,6 +533,119 @@ private boolean doesClusterExist(String cluster) {
return ZKUtil.isClusterSetup(cluster, zkClient);
}
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound("Cluster is not properly setup!");
+}
+
+HelixAdmin admin = getHelixAdmin();
+ZNRecord record;
+try {
+ record = toZNRecord(content);
+} catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ",
Exception: " + e);
+ return badRequest("Input is not a vaild ZNRecord!");
+}
+
+try {
+ CloudConfig cloudConfig = new CloudConfig.Builder(record).build();
+ admin.addCloudConfig(clusterId, cloudConfig);
+} catch (HelixException ex) {
+ _logger.error("Error in adding a CloudConfig to cluster: " + clusterId,
ex);
+ return badRequest(ex.getMessage());
+} catch (Exception ex) {
+ _logger.error("Cannot add CloudConfig to cluster: " + clusterId, ex);
+ return badRequest(ex.getMessage());
Review comment:
This is not badRequest. It may be something related to internal server
error.
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] i3wangyi commented on a change in pull request #682: [HELIX-2497] Implement sharding key prefix-tree class
i3wangyi commented on a change in pull request #682: [HELIX-2497] Implement
sharding key prefix-tree class
URL: https://github.com/apache/helix/pull/682#discussion_r367112058
##
File path: helix-core/src/main/java/org/apache/helix/ShardingKeyTreeNode.java
##
@@ -0,0 +1,74 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+
+public class ShardingKeyTreeNode {
Review comment:
TrieNode more exactly, isn't 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] NealSun96 opened a new pull request #682: [HELIX-2497] Implement sharding key prefix-tree class
NealSun96 opened a new pull request #682: [HELIX-2497] Implement sharding key
prefix-tree class
URL: https://github.com/apache/helix/pull/682
### Description
- [x] Here are some details about my PR, including screenshots of any UI
changes:
Created a class that represents the prefix-tree nodes used for sharding
keys.
### Tests
- [x] The following is the result of the "mvn test" command on the
appropriate module:
[INFO] Tests run: 897, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
3,277.825 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 897, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] BUILD SUCCESS
[INFO]
[INFO] Total time: 54:42 min
[INFO] Finished at: 2020-01-15T11:49:29-08:00
[INFO]
### 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] narendly commented on a change in pull request #671: Add ZooKeeperAccessor to helix-rest
narendly commented on a change in pull request #671: Add ZooKeeperAccessor to
helix-rest
URL: https://github.com/apache/helix/pull/671#discussion_r367057698
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
##
@@ -0,0 +1,181 @@
+package org.apache.helix.rest.server.resources.zookeeper;
+
+/*
+ * 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.UnsupportedEncodingException;
+import java.util.Base64;
+import java.util.List;
+import java.util.Map;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+
+import com.google.common.collect.ImmutableMap;
+import org.apache.helix.AccessOption;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.rest.common.ContextPropertyKeys;
+import org.apache.helix.rest.server.ServerContext;
+import org.apache.helix.rest.server.resources.AbstractResource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * ZooKeeperAccessor provides methods for accessing ZooKeeper resources
(ZNodes).
+ * It provides basic ZooKeeper features supported by ZkClient.
+ */
+@Path("/zookeeper")
+public class ZooKeeperAccessor extends AbstractResource {
+ private static final Logger LOG =
LoggerFactory.getLogger(ZooKeeperAccessor.class.getName());
+ private ZkBaseDataAccessor _zkBaseDataAccessor;
+
+ public enum ZooKeeperCommand {
+exists, getBinaryData, getStringData, getChildren
+ }
+
+ @GET
+ @Path("{path: .+}")
+ public Response get(@PathParam("path") String path, @QueryParam("command")
String commandStr) {
+ZooKeeperCommand cmd;
+try {
+ cmd = ZooKeeperCommand.valueOf(commandStr);
+} catch (Exception e) {
+ return badRequest("Invalid ZooKeeper command: " + commandStr);
+}
+
+// Lazily initialize ZkBaseDataAccessor
+ServerContext _serverContext =
+(ServerContext)
_application.getProperties().get(ContextPropertyKeys.SERVER_CONTEXT.name());
+_zkBaseDataAccessor = _serverContext.getByteArrayZkBaseDataAccessor();
+
+// Need to prepend a "/" since JAX-RS regex removes it
+path = "/" + path;
+switch (cmd) {
+ case exists:
+return exists(path);
+ case getBinaryData:
+return getData(path, cmd);
+ case getStringData:
+return getData(path, cmd);
Review comment:
Split it into two different methods.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #671: Add ZooKeeperAccessor to helix-rest
narendly commented on a change in pull request #671: Add ZooKeeperAccessor to
helix-rest
URL: https://github.com/apache/helix/pull/671#discussion_r367058485
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java
##
@@ -122,27 +122,34 @@ public HelixDataAccessor getDataAccssor(String
clusterName) {
}
}
- public ZkBaseDataAccessor getByteArrayBaseDataAccessor() {
-if (_byteArrayBaseDataAccessor == null) {
- synchronized (this) {
-if (_byteArrayBaseDataAccessor == null) {
- _byteArrayBaseDataAccessor = new ZkBaseDataAccessor<>(_zkAddr, new
ZkSerializer() {
-@Override
-public byte[] serialize(Object o)
-throws ZkMarshallingError {
- throw new UnsupportedOperationException("Serialize is not
supported yet!");
-}
-
-@Override
-public Object deserialize(byte[] bytes)
-throws ZkMarshallingError {
- return bytes;
-}
- });
-}
+ /**
+ * Returns a lazily-instantiated ZkBaseDataAccessor for the byte array type.
+ * @return
+ */
+ public ZkBaseDataAccessor getByteArrayZkBaseDataAccessor() {
+ZkBaseDataAccessor byteArrayZkBaseDataAccessor =
_byteArrayZkBaseDataAccessor;
+if (byteArrayZkBaseDataAccessor != null) { // First check (no locking)
+ return byteArrayZkBaseDataAccessor;
+}
+
+synchronized (this) {
+ if (_byteArrayZkBaseDataAccessor == null) { // Second check (with
locking)
+_byteArrayZkBaseDataAccessor = new ZkBaseDataAccessor<>(_zkAddr, new
ZkSerializer() {
+ @Override
+ public byte[] serialize(Object o)
+ throws ZkMarshallingError {
+throw new UnsupportedOperationException("serialize() is not
supported yet!");
Review comment:
This will be supported at some point. I will change the log message, but add
a TODO 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] narendly commented on a change in pull request #671: Add ZooKeeperAccessor to helix-rest
narendly commented on a change in pull request #671: Add ZooKeeperAccessor to
helix-rest
URL: https://github.com/apache/helix/pull/671#discussion_r367052988
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/zookeeper/ZooKeeperAccessor.java
##
@@ -0,0 +1,176 @@
+package org.apache.helix.rest.server.resources.zookeeper;
+
+/*
+ * 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.UnsupportedEncodingException;
+import java.util.Base64;
+import java.util.List;
+import java.util.Map;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+
+import com.google.common.base.Enums;
+import com.google.common.collect.ImmutableMap;
+import org.apache.helix.AccessOption;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.rest.common.ContextPropertyKeys;
+import org.apache.helix.rest.server.ServerContext;
+import org.apache.helix.rest.server.resources.AbstractResource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * ZooKeeperAccessor provides methods for accessing ZooKeeper resources
(ZNodes).
+ * It provides basic ZooKeeper features supported by ZkClient.
+ */
+@Path("/zookeeper")
+public class ZooKeeperAccessor extends AbstractResource {
+ private static final Logger LOG =
LoggerFactory.getLogger(ZooKeeperAccessor.class.getName());
+ private ZkBaseDataAccessor _zkBaseDataAccessor;
Review comment:
Sure.
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 #671: Add ZooKeeperAccessor to helix-rest
narendly commented on a change in pull request #671: Add ZooKeeperAccessor to helix-rest URL: https://github.com/apache/helix/pull/671#discussion_r367058292 ## File path: helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java ## @@ -47,8 +47,8 @@ private ZKHelixAdmin _zkHelixAdmin; private ClusterSetup _clusterSetup; private ConfigAccessor _configAccessor; - // The lazy initialized base data accessor that reads/writes byte array to ZK - private ZkBaseDataAccessor _byteArrayBaseDataAccessor; + // A lazily-initialized base data accessor that reads/writes byte array to ZK Review comment: I don't think this is necessary at this point. We make it clear with an unsupported exception and I don't think it's a good idea to include "unsupported" or readOnly in the name since we may support it in the future. 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] jackjlli commented on issue #680: Question on CrushEd rebalance algorithm and MasterSalve state model for Pinot
jackjlli commented on issue #680: Question on CrushEd rebalance algorithm and MasterSalve state model for Pinot URL: https://github.com/apache/helix/issues/680#issuecomment-574809553 This is the workaround regarding to this issue: https://github.com/apache/incubator-pinot/pull/4985 The idea is that, before the first Pinot controller full starts up, the leadControllerResource won't turn on FULL-AUTO rebalance strategy. We'll also wait for Helix team to uniform the algorithm behaviors before closing this issue. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: 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] i3wangyi commented on a change in pull request #681: Integration test for controller connect and disconnect
i3wangyi commented on a change in pull request #681: Integration test for controller connect and disconnect URL: https://github.com/apache/helix/pull/681#discussion_r367036148 ## File path: helix-core/src/main/java/org/apache/helix/HelixTimerTask.java ## @@ -32,4 +32,9 @@ * Stop a timer task */ public abstract void stop(); + + /** + * Validate if the timer task is stopped + */ + public abstract boolean isStopped(); Review comment: The only exposed interface from ZkHelixManager is `List _controllerTimerTasks = xxx` and the actual implementation class is `static class StatusDumpTask` which is not exposed neither. How could I create a protected method and make it only visible for the test? 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] i3wangyi commented on a change in pull request #681: Integration test for controller connect and disconnect
i3wangyi commented on a change in pull request #681: Integration test for
controller connect and disconnect
URL: https://github.com/apache/helix/pull/681#discussion_r367034758
##
File path:
helix-core/src/test/java/org/apache/helix/integration/controller/TestControllerLeadershipChange.java
##
@@ -20,29 +20,136 @@
*/
import java.lang.management.ManagementFactory;
+import java.util.List;
import javax.management.MBeanServer;
import javax.management.ObjectName;
import org.apache.helix.AccessOption;
import org.apache.helix.HelixDataAccessor;
import org.apache.helix.HelixManager;
import org.apache.helix.HelixManagerFactory;
+import org.apache.helix.HelixTimerTask;
import org.apache.helix.InstanceType;
import org.apache.helix.PropertyPathBuilder;
import org.apache.helix.common.ZkTestBase;
+import org.apache.helix.integration.manager.ClusterControllerManager;
import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.CallbackHandler;
+import org.apache.helix.manager.zk.client.HelixZkClient;
import org.apache.helix.model.IdealState;
import org.apache.helix.model.LiveInstance;
import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
import
org.apache.helix.tools.ClusterVerifiers.BestPossibleExternalViewVerifier;
import org.apache.helix.tools.ClusterVerifiers.ZkHelixClusterVerifier;
import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
+
+/**
+ * Integration test on controller leadership on several phases given the test
cluster:
+ * 1. When a standby node becomes the leader node
+ * - single leader node mode
+ * 2. When a leader node becomes standby node
+ * - single leader node mode
+ * 3. When a leader node becomes standby node and the other leader node
becomes leader node
+ * - The dual leader nodes mode when the leadership of the test cluster
gets changed
+ */
public class TestControllerLeadershipChange extends ZkTestBase {
+ private final String CLASS_NAME = getShortClassName();
+ private final String CLUSTER_NAME = "TestCluster-" + CLASS_NAME;
+
+ @BeforeClass
+ public void beforeClass()
+ throws Exception {
+super.beforeClass();
+_gSetupTool.addCluster(CLUSTER_NAME, true);
+// add some instances
+_gSetupTool.addInstanceToCluster(CLUSTER_NAME, "TestInstance");
+_gSetupTool.addResourceToCluster(CLUSTER_NAME, "TestResource", 10,
"MasterSlave");
+ }
@Test
- public void testMissingTopStateDurationMonitoring() throws Exception {
+ public void testOnSingleController() {
Review comment:
Thanks for mentioning it! I was unaware of the existing test case at all
(it's not trivial to find out). Will merge the similarities part later
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] jiajunwang commented on issue #597: Bump jackson-databind from 2.9.5 to 2.9.10.1 in /helix-rest
jiajunwang commented on issue #597: Bump jackson-databind from 2.9.5 to 2.9.10.1 in /helix-rest URL: https://github.com/apache/helix/pull/597#issuecomment-574786584 @narendly Could help to confirm if we want to do this upgrade? Moreover, I didn't see this item in the ivy file. Not sure if it will become a potential problem? 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] jiajunwang closed pull request #558: New module hosting Helix Zookeeper access logic
jiajunwang closed pull request #558: New module hosting Helix Zookeeper access logic URL: https://github.com/apache/helix/pull/558 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] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366734655
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,175 @@
+package org.apache.helix;
+
+/*
+ * 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.InputStream;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Hold helix cloud properties read from CloudConfig and user defined files.
Clients may override the fields from their application.
+ */
+public class HelixCloudProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixCloudProperty.class.getName());
+ private static final String AZURE_CLOUD_PROPERTY_FILE =
SystemPropertyKeys.AZURE_CLOUD_PROPERTIES;
+ private static final String CLOUD_INFO_SOURCE = "cloud_info_source";
+ private static final String CLOUD_INFO_PROCESSFOR_NAME =
"cloud_info_processor_name";
+ private static final String CLOUD_MAX_RETRY = "cloud_max_retry";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ // Denote whether the instance is considered as in a cloud environment.
+ private boolean _isCloudEnabled;
+
+ // Unique id of the cloud environment where the instance is in.
+ private String _cloudId;
+
+ // Cloud environment provider, e.g. Azure, AWS, GCP, etc.
+ private String _cloudProvider;
+
+ // The sources where the cloud instance information can be retrieved from.
+ private List _cloudInfoSources;
+
+ // The name of the function that will fetch and parse cloud instance
information.
+ private String _cloudInfoProcessorName;
+
+ // Http max retry times when querying the cloud instance information from
cloud environment.
+ private int _cloudMaxRetry;
+
+ // Http connection time when querying the cloud instance information from
cloud environment.
+ private long _cloudConnectionTimeout;
+
+ // Http request timeout when querying the cloud instance information from
cloud environment.
+ private long _cloudRequestTimeout;
+
+ // Other customized properties that may be used.
+ private Properties _customizedCloudProperties = new Properties();
+
+ /**
+ * Initialize Helix Cloud Property based on the provider
+ * @param
+ */
+ public HelixCloudProperty(CloudConfig cloudConfig) {
+setCloudEndabled(cloudConfig.isCloudEnabled());
+setCloudId(cloudConfig.getCloudID());
+setCloudProvider(cloudConfig.getCloudProvider());
+switch (CloudProvider.valueOf(cloudConfig.getCloudProvider())) {
+ case AZURE:
+Properties azureProperties = new Properties();
+try {
+ InputStream stream =
+
Thread.currentThread().getContextClassLoader().getResourceAsStream(AZURE_CLOUD_PROPERTY_FILE);
+ azureProperties.load(stream);
+} catch (Exception e) {
+ String errMsg = "failed to open Helix Azure cloud properties file: "
+ AZURE_CLOUD_PROPERTY_FILE;
+ throw new IllegalArgumentException(errMsg, e);
+}
+LOG.info("Successfully loaded Helix Azure cloud properties: " +
azureProperties);
+
setCloudInfoSources(Collections.singletonList(azureProperties.getProperty(CLOUD_INFO_SOURCE)));
+
setCloudInfoProcessorName(azureProperties.getProperty(CLOUD_INFO_PROCESSFOR_NAME));
+setCloudMaxRetry(azureProperties.getProperty(CLOUD_MAX_RETRY));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
+ case CUSTOMIZED:
+setCloudInfoSources(cloudConfig.getCloudInfoSources());
+setCloudInfoProcessorName(cloudConfig.getCloudInfoProcessorName());
Review comment:
break
This is an automated message from the Apache Git Service.
To respond to the
[GitHub] [helix] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366734722
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,175 @@
+package org.apache.helix;
+
+/*
+ * 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.InputStream;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Hold helix cloud properties read from CloudConfig and user defined files.
Clients may override the fields from their application.
+ */
+public class HelixCloudProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixCloudProperty.class.getName());
+ private static final String AZURE_CLOUD_PROPERTY_FILE =
SystemPropertyKeys.AZURE_CLOUD_PROPERTIES;
+ private static final String CLOUD_INFO_SOURCE = "cloud_info_source";
+ private static final String CLOUD_INFO_PROCESSFOR_NAME =
"cloud_info_processor_name";
+ private static final String CLOUD_MAX_RETRY = "cloud_max_retry";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ // Denote whether the instance is considered as in a cloud environment.
+ private boolean _isCloudEnabled;
+
+ // Unique id of the cloud environment where the instance is in.
+ private String _cloudId;
+
+ // Cloud environment provider, e.g. Azure, AWS, GCP, etc.
+ private String _cloudProvider;
+
+ // The sources where the cloud instance information can be retrieved from.
+ private List _cloudInfoSources;
+
+ // The name of the function that will fetch and parse cloud instance
information.
+ private String _cloudInfoProcessorName;
+
+ // Http max retry times when querying the cloud instance information from
cloud environment.
+ private int _cloudMaxRetry;
+
+ // Http connection time when querying the cloud instance information from
cloud environment.
+ private long _cloudConnectionTimeout;
+
+ // Http request timeout when querying the cloud instance information from
cloud environment.
+ private long _cloudRequestTimeout;
+
+ // Other customized properties that may be used.
+ private Properties _customizedCloudProperties = new Properties();
+
+ /**
+ * Initialize Helix Cloud Property based on the provider
+ * @param
+ */
+ public HelixCloudProperty(CloudConfig cloudConfig) {
+setCloudEndabled(cloudConfig.isCloudEnabled());
+setCloudId(cloudConfig.getCloudID());
+setCloudProvider(cloudConfig.getCloudProvider());
+switch (CloudProvider.valueOf(cloudConfig.getCloudProvider())) {
+ case AZURE:
+Properties azureProperties = new Properties();
+try {
+ InputStream stream =
+
Thread.currentThread().getContextClassLoader().getResourceAsStream(AZURE_CLOUD_PROPERTY_FILE);
+ azureProperties.load(stream);
+} catch (Exception e) {
+ String errMsg = "failed to open Helix Azure cloud properties file: "
+ AZURE_CLOUD_PROPERTY_FILE;
+ throw new IllegalArgumentException(errMsg, e);
+}
+LOG.info("Successfully loaded Helix Azure cloud properties: " +
azureProperties);
+
setCloudInfoSources(Collections.singletonList(azureProperties.getProperty(CLOUD_INFO_SOURCE)));
+
setCloudInfoProcessorName(azureProperties.getProperty(CLOUD_INFO_PROCESSFOR_NAME));
+setCloudMaxRetry(azureProperties.getProperty(CLOUD_MAX_RETRY));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
+ case CUSTOMIZED:
+setCloudInfoSources(cloudConfig.getCloudInfoSources());
+setCloudInfoProcessorName(cloudConfig.getCloudInfoProcessorName());
+ default:
+LOG.info("Unsupported cloud provider: " +
cloudConfig.getCloudProvider());
Review comment:
Exception?
[GitHub] [helix] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366730747
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -76,9 +77,16 @@
final StateMachineEngine _stateMachineEngine;
final LiveInstanceInfoProvider _liveInstanceInfoProvider;
final List _preConnectCallbacks;
+ final HelixParticipantProperty _helixParticipantProperty;
public ParticipantManager(HelixManager manager, HelixZkClient zkclient, int
sessionTimeout,
LiveInstanceInfoProvider liveInstanceInfoProvider,
List preConnectCallbacks) {
+this(manager, zkclient, sessionTimeout, liveInstanceInfoProvider,
preConnectCallbacks, null);
Review comment:
I agree with Hunter that it would be better to put an empty property if
possible. Easier for us to handle the conditions. But it is optional,
especially if you mark this constructor as deprecated.
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] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366734574
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,175 @@
+package org.apache.helix;
+
+/*
+ * 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.InputStream;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Hold helix cloud properties read from CloudConfig and user defined files.
Clients may override the fields from their application.
+ */
+public class HelixCloudProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixCloudProperty.class.getName());
+ private static final String AZURE_CLOUD_PROPERTY_FILE =
SystemPropertyKeys.AZURE_CLOUD_PROPERTIES;
+ private static final String CLOUD_INFO_SOURCE = "cloud_info_source";
+ private static final String CLOUD_INFO_PROCESSFOR_NAME =
"cloud_info_processor_name";
+ private static final String CLOUD_MAX_RETRY = "cloud_max_retry";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ // Denote whether the instance is considered as in a cloud environment.
+ private boolean _isCloudEnabled;
+
+ // Unique id of the cloud environment where the instance is in.
+ private String _cloudId;
+
+ // Cloud environment provider, e.g. Azure, AWS, GCP, etc.
+ private String _cloudProvider;
+
+ // The sources where the cloud instance information can be retrieved from.
+ private List _cloudInfoSources;
+
+ // The name of the function that will fetch and parse cloud instance
information.
+ private String _cloudInfoProcessorName;
+
+ // Http max retry times when querying the cloud instance information from
cloud environment.
+ private int _cloudMaxRetry;
+
+ // Http connection time when querying the cloud instance information from
cloud environment.
+ private long _cloudConnectionTimeout;
+
+ // Http request timeout when querying the cloud instance information from
cloud environment.
+ private long _cloudRequestTimeout;
+
+ // Other customized properties that may be used.
+ private Properties _customizedCloudProperties = new Properties();
+
+ /**
+ * Initialize Helix Cloud Property based on the provider
+ * @param
+ */
+ public HelixCloudProperty(CloudConfig cloudConfig) {
+setCloudEndabled(cloudConfig.isCloudEnabled());
+setCloudId(cloudConfig.getCloudID());
+setCloudProvider(cloudConfig.getCloudProvider());
+switch (CloudProvider.valueOf(cloudConfig.getCloudProvider())) {
+ case AZURE:
+Properties azureProperties = new Properties();
+try {
+ InputStream stream =
+
Thread.currentThread().getContextClassLoader().getResourceAsStream(AZURE_CLOUD_PROPERTY_FILE);
+ azureProperties.load(stream);
+} catch (Exception e) {
+ String errMsg = "failed to open Helix Azure cloud properties file: "
+ AZURE_CLOUD_PROPERTY_FILE;
+ throw new IllegalArgumentException(errMsg, e);
+}
+LOG.info("Successfully loaded Helix Azure cloud properties: " +
azureProperties);
+
setCloudInfoSources(Collections.singletonList(azureProperties.getProperty(CLOUD_INFO_SOURCE)));
+
setCloudInfoProcessorName(azureProperties.getProperty(CLOUD_INFO_PROCESSFOR_NAME));
+setCloudMaxRetry(azureProperties.getProperty(CLOUD_MAX_RETRY));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
Review comment:
break
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:
[GitHub] [helix] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366731391
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -202,6 +205,11 @@ public ZKHelixManager(String clusterName, String
instanceName, InstanceType inst
public ZKHelixManager(String clusterName, String instanceName, InstanceType
instanceType,
String zkAddress, HelixManagerStateListener stateListener) {
+this(clusterName, instanceName, instanceType, zkAddress,stateListener,
null);
Review comment:
You are doing "_helixManagerProperty =
HelixPropertyFactory.getInstance().getHelixManagerProperty(zkAddress,
clusterName);" anyway later, why not do it here? So in the other constructor,
you don't need to check null. Or if null is input we can just throw an
exception.
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] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366735055
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,175 @@
+package org.apache.helix;
+
+/*
+ * 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.InputStream;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Hold helix cloud properties read from CloudConfig and user defined files.
Clients may override the fields from their application.
+ */
+public class HelixCloudProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixCloudProperty.class.getName());
+ private static final String AZURE_CLOUD_PROPERTY_FILE =
SystemPropertyKeys.AZURE_CLOUD_PROPERTIES;
+ private static final String CLOUD_INFO_SOURCE = "cloud_info_source";
+ private static final String CLOUD_INFO_PROCESSFOR_NAME =
"cloud_info_processor_name";
+ private static final String CLOUD_MAX_RETRY = "cloud_max_retry";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ // Denote whether the instance is considered as in a cloud environment.
+ private boolean _isCloudEnabled;
+
+ // Unique id of the cloud environment where the instance is in.
+ private String _cloudId;
+
+ // Cloud environment provider, e.g. Azure, AWS, GCP, etc.
+ private String _cloudProvider;
+
+ // The sources where the cloud instance information can be retrieved from.
+ private List _cloudInfoSources;
+
+ // The name of the function that will fetch and parse cloud instance
information.
+ private String _cloudInfoProcessorName;
+
+ // Http max retry times when querying the cloud instance information from
cloud environment.
+ private int _cloudMaxRetry;
+
+ // Http connection time when querying the cloud instance information from
cloud environment.
+ private long _cloudConnectionTimeout;
+
+ // Http request timeout when querying the cloud instance information from
cloud environment.
+ private long _cloudRequestTimeout;
+
+ // Other customized properties that may be used.
+ private Properties _customizedCloudProperties = new Properties();
+
+ /**
+ * Initialize Helix Cloud Property based on the provider
+ * @param
+ */
+ public HelixCloudProperty(CloudConfig cloudConfig) {
+setCloudEndabled(cloudConfig.isCloudEnabled());
+setCloudId(cloudConfig.getCloudID());
+setCloudProvider(cloudConfig.getCloudProvider());
+switch (CloudProvider.valueOf(cloudConfig.getCloudProvider())) {
+ case AZURE:
+Properties azureProperties = new Properties();
+try {
+ InputStream stream =
+
Thread.currentThread().getContextClassLoader().getResourceAsStream(AZURE_CLOUD_PROPERTY_FILE);
+ azureProperties.load(stream);
+} catch (Exception e) {
+ String errMsg = "failed to open Helix Azure cloud properties file: "
+ AZURE_CLOUD_PROPERTY_FILE;
+ throw new IllegalArgumentException(errMsg, e);
+}
+LOG.info("Successfully loaded Helix Azure cloud properties: " +
azureProperties);
+
setCloudInfoSources(Collections.singletonList(azureProperties.getProperty(CLOUD_INFO_SOURCE)));
+
setCloudInfoProcessorName(azureProperties.getProperty(CLOUD_INFO_PROCESSFOR_NAME));
+setCloudMaxRetry(azureProperties.getProperty(CLOUD_MAX_RETRY));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
+ case CUSTOMIZED:
+setCloudInfoSources(cloudConfig.getCloudInfoSources());
+setCloudInfoProcessorName(cloudConfig.getCloudInfoProcessorName());
+ default:
+LOG.info("Unsupported cloud provider: " +
cloudConfig.getCloudProvider());
+}
+ }
+
+ public boolean getCloudEnabled() {
+return
[GitHub] [helix] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366736050
##
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##
@@ -0,0 +1,75 @@
+package org.apache.helix;
+
+/*
+ * 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.Properties;
+import org.apache.helix.model.CloudConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Hold Helix participant properties. The participant properties further hold
Helix cloud properties and some other properties
+ * specific for the participant.
+ */
+public class HelixManagerProperty {
+ private static final Logger LOG =
LoggerFactory.getLogger(HelixManagerProperty.class.getName());
+ private String _version;
+ private long _healthReportLatency;
+ private HelixCloudProperty _helixCloudProperty;
+
+ /**
+ * Initialize Helix manager property with default value
+ * @param properties helix manager related properties input as a map
+ * @param cloudConfig cloudConfig read from Zookeeper
+ */
+ public HelixManagerProperty(Properties properties, CloudConfig cloudConfig) {
+if (cloudConfig != null) {
+ _helixCloudProperty = new HelixCloudProperty(cloudConfig);
Review comment:
I think we discussed this once, if we have a boolean "isInCloud" (I think we
had a better name, but I don't remember), then this nullable value can be
handled more gracefully.
At least we can check if the "isInCloud" == true, null cloudconfig input
should trigger an exception.
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] jiajunwang commented on a change in pull request #653: add Helix properties factory and class
jiajunwang commented on a change in pull request #653: add Helix properties
factory and class
URL: https://github.com/apache/helix/pull/653#discussion_r366733903
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkClusterManager.java
##
@@ -63,18 +63,11 @@ public void testController() throws Exception {
_gZkClient.deleteRecursively("/" + clusterName);
}
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+
ZKHelixManager controller =
new ZKHelixManager(clusterName, null, InstanceType.CONTROLLER,
ZK_ADDR);
-try {
- controller.connect();
- Assert.fail("Should throw HelixException if initial cluster structure is
not setup");
Review comment:
Why this test logic is removed? We don't throw exceptions on empty cluster
structure anymore?
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
