[GitHub] [helix] pkuwm commented on issue #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on issue #642: Fix handleNewSession creating ephemeral node with expired session URL: https://github.com/apache/helix/pull/642#issuecomment-571894621 Test results: [INFO] Results: [INFO] [ERROR] Failures: [ERROR] TestAddNodeAfterControllerStart.testDistributed:165 » ThreadTimeout Method org... [ERROR] TestJobQueueCleanUp.testJobQueueAutoCleanUp » ThreadTimeout Method org.testng [INFO] [ERROR] Tests run: 896, Failures: 2, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD FAILURE [INFO] [INFO] Total time: 01:05 h [INFO] Finished at: 2020-01-07T18:48:16-08:00 [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.891 s - in org.apache.helix.integration.TestAddNodeAfterControllerStart [INFO] [INFO] Results: [INFO] [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 12.397 s [INFO] Finished at: 2020-01-07T21:02:37-08:00 [INFO] [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.03 s - in org.apache.helix.integration.task.TestJobQueueCleanUp [INFO] [INFO] Results: [INFO] [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 14.887 s [INFO] Finished at: 2020-01-07T21:13:52-08:00 [INFO] 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 #673: Add Helix Distributed lock module
pkuwm commented on a change in pull request #673: Add Helix Distributed lock
module
URL: https://github.com/apache/helix/pull/673#discussion_r364060589
##
File path: helix-lock/pom.xml
##
@@ -0,0 +1,152 @@
+
+
+http://maven.apache.org/POM/4.0.0;
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+
+org.apache.helix
+helix
+0.9.2-SNAPSHOT
+
+ 4.0.0
+
+ helix-lock
+ bundle
+ Apache Helix :: Distributed Lock
+
+
+
+ org.apache.helix*,
+ org.slf4j*;version="[1.6,2)",
+ *
+
+
org.apache.helix.lock*;version="${project.version};-noimport:=true
+
+
+
+
+ org.yaml
+ snakeyaml
+ 1.17
+
+
+ org.apache.helix
+ helix-core
+
+
+ org.apache.commons
+ commons-lang3
+ 3.8.1
+
+
+ org.apache.httpcomponents
+ httpclient
+ 4.5.8
+
+
+ org.eclipse.jetty
+ jetty-server
+ 9.1.0.RC0
Review comment:
I see that the parent pom also has jetty-server dependency:
```
org.eclipse.jetty
jetty-server
8.1.8.v20121106
```
Is there any reason you want to use this specific version(9.1.0.RC0)? To me,
8.1.8.v20121106 is pretty old. Not sure if it is time to upgrade this
dependency in the parent pom, too.
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 opened a new issue #676: Add method to return session ID in zkclient waitUntilConnected
pkuwm opened a new issue #676: Add method to return session ID in zkclient waitUntilConnected URL: https://github.com/apache/helix/issues/676 Current zkclient's waitUntilConnected does not return a zk session ID. zkClient's getSessionId() could bring in session race condition: session A is connected in waitUntilConnected, but when zkClient.getSession() is called in zkHelixManager, session A might be already expired and so zkClient.getSession() gets session B. This session ID is critical for the firs time handling new session after zkclient is created in zkHelixManager. 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 #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on a change in pull request #642: Fix handleNewSession creating
ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364029981
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -1134,9 +1159,31 @@ public void handleNewSession(final String sessionId)
throws Exception {
// Wait until we get a non-zero session id. Otherwise, getSessionId()
might be null.
waitUntilConnected();
-// TODO: filter out stale sessions here.
+/*
+ * Filter out stale sessions. If a session id is not null and not the same
as current session
+ * id, this session is expired. With this filtering, expired sessions are
NOT handled,
+ * so performance is expected to improve.
+ */
+if (sessionId != null && !getSessionId().equals(sessionId)) {
+ LOG.info("Session is expired and not handled. Expected: {}. Actual:
{}.", sessionId,
Review comment:
Resolved.
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 #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on a change in pull request #642: Fix handleNewSession creating
ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364029827
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -178,89 +194,18 @@ private void createLiveInstance() {
}
}
-boolean retry;
-do {
- retry = false;
- try {
-_zkclient.createEphemeral(liveInstancePath, liveInstance.getRecord());
-LOG.info("LiveInstance created, path: " + liveInstancePath + ",
sessionId: " + liveInstance.getEphemeralOwner());
- } catch (ZkNodeExistsException e) {
Review comment:
The wait and retry logic is kept.
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 #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on a change in pull request #642: Fix handleNewSession creating
ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364029927
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -697,10 +699,19 @@ void createClient() throws Exception {
int retryCount = 0;
while (retryCount < 3) {
try {
-// TODO: synchronize this block and wait for the new non-zero session
ID updated.
-_zkclient.waitUntilConnected(_connectionInitTimeout,
TimeUnit.MILLISECONDS);
+// TODO: get session id from waitUntilConnected to avoid race condition
Review comment:
Will submit a new PR for this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on a change in pull request #642: Fix handleNewSession creating
ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364030010
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -1134,9 +1159,31 @@ public void handleNewSession(final String sessionId)
throws Exception {
// Wait until we get a non-zero session id. Otherwise, getSessionId()
might be null.
waitUntilConnected();
-// TODO: filter out stale sessions here.
+/*
+ * Filter out stale sessions. If a session id is not null and not the same
as current session
+ * id, this session is expired. With this filtering, expired sessions are
NOT handled,
+ * so performance is expected to improve.
+ */
+if (sessionId != null && !getSessionId().equals(sessionId)) {
+ LOG.info("Session is expired and not handled. Expected: {}. Actual:
{}.", sessionId,
+ getSessionId());
+ return;
+}
+
+/*
+ * When a null session id is passed in, we will take current session's id
for following
+ * operations. Please note that current session might not be the one we
expect to handle,
+ * because the one we expect might be already expired when the zk event is
waiting in the
+ * event queue. Why we use current session here is for backward
compatibility with the old
+ * method handleNewSession().
+ */
+if (sessionId == null) {
+ sessionId = getSessionId();
+ LOG.warn("Session id: is passed in. Current session id: {} will
be used.", sessionId);
Review comment:
Resolved
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 #642: Fix handleNewSession creating ephemeral node with expired session
pkuwm commented on a change in pull request #642: Fix handleNewSession creating
ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364024171
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -97,10 +98,25 @@ public ParticipantManager(HelixManager manager,
HelixZkClient zkclient, int sess
}
/**
- * Handle new session for a participang.
- * @throws Exception
+ * Handles a new session for a participant. The new session's id is passed
in when participant
+ * manager is created, as it is required in
+ * {@link
org.apache.helix.manager.zk.zookeeper.ZkClient#createEphemeral(String, Object,
String)}
Review comment:
Sure. Will address this link.
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_r364025904
##
File path:
helix-core/src/main/java/org/apache/helix/HelixParticipantProperty.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.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 HelixParticipantProperty {
Review comment:
Same for the set methods, is private good enough?
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_r364025530
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,158 @@
+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 RETRY_MAX = "retry_max";
+ private static final String CONNECTION_TIMEOUT_MS = "connection_timeout_ms";
+ private static final String REQUEST_TIMEOUT_MS = "request_timeout_ms";
+
+ private boolean _isCloudEnabled;
+ private String _cloudId;
+ private String _cloudProvider;
+ private List _cloudInfoSources;
+ private String _cloudInfoProcessorName;
+ private int _maxRetry;
+ private long _cloudConnectionTimeout;
+ private long _cloudRequestTimeout;
+ private Properties _customizedProperties = 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(RETRY_MAX));
+
setCloudConnectionTimeout(azureProperties.getProperty(CONNECTION_TIMEOUT_MS));
+
setCloudRequestTimeout(azureProperties.getProperty(REQUEST_TIMEOUT_MS));
+ case CUSTOMIZED:
+setCloudInfoSources(cloudConfig.getCloudInfoSources());
+setCloudInfoProcessorName(cloudConfig.getCloudInfoProcessorName());
+ default:
+LOG.info("unrecognized cloud provider: " +
cloudConfig.getCloudProvider());
Review comment:
Unsupported cloud provider ?
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_r364026530
##
File path:
helix-core/src/main/java/org/apache/helix/api/cloud/CloudInstanceInformation.java
##
@@ -0,0 +1,40 @@
+package org.apache.helix.api.cloud;
+
+/*
+ * 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.
+ */
+
+/**
+ * Generic interface for cloud instance information
+ */
+public interface CloudInstanceInformation {
+ /**
+ * Get the the value of a specific cloud instance field by name
+ * @return the value of the field
+ */
+ String get(String key);
Review comment:
Shouldn't it be String get(CloudInstanceField key); ? Otherwise, what's the
point of having the enum 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] 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_r364027631 ## File path: helix-core/src/main/resources/azure-cloud.properties ## @@ -0,0 +1,25 @@ +# +# 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. +# + +cloud_info_resource = "http://169.254.169.254/metadata/instance?api-version=2019-06-04; Review comment: Is this for testing? Please comment if this will be the default value for all of our customers. 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_r364023727
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,158 @@
+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";
Review comment:
Should we use capitalized letters for consistency?
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_r364027213
##
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);
+ }
+
+ public ZKHelixManager(String clusterName, String instanceName, InstanceType
instanceType,
+ String zkAddress, HelixManagerStateListener stateListener,
HelixParticipantProperty helixParticipantProperty) {
Review comment:
And we might want to deprecate the older constructor.
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_r364025708
##
File path: helix-core/src/main/java/org/apache/helix/HelixCloudProperty.java
##
@@ -0,0 +1,158 @@
+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 {
Review comment:
Shall we make all the set methods to be private if they are not used
anywhere else?
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_r364027114
##
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);
+ }
+
+ public ZKHelixManager(String clusterName, String instanceName, InstanceType
instanceType,
+ String zkAddress, HelixManagerStateListener stateListener,
HelixParticipantProperty helixParticipantProperty) {
Review comment:
HelixManagerProperty would be better.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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_r364026705
##
File path:
helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformation.java
##
@@ -0,0 +1,77 @@
+package org.apache.helix.cloud.azure;
+
+/*
+ * 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.Map;
+
+import org.apache.helix.api.cloud.CloudInstanceInformation;
+
+
+public class AzureCloudInstanceInformation implements CloudInstanceInformation
{
+ private Map _cloudInstanceInfoMap;
+
+ /**
+ * Instantiate the AzureCloudInstanceInformation using each field
individually.
+ * Users should use AzureCloudInstanceInformation.Builder to create
information.
+ * @param cloudInstanceInfoMap
+ */
+ protected AzureCloudInstanceInformation(Map
cloudInstanceInfoMap) {
+_cloudInstanceInfoMap = cloudInstanceInfoMap;
+ }
+
+ @Override
+ public String get(String key) {
+return _cloudInstanceInfoMap.get(key);
+ }
+
+ public static class Builder {
+private Map _cloudInstanceInfoMap = null;
+
+public AzureCloudInstanceInformation build() {
+ return new AzureCloudInstanceInformation(_cloudInstanceInfoMap);
+}
+
+/**
+ * Default constructor
+ */
+public Builder() {
+}
+
+public Builder setInstanceName(String v) {
Review comment:
Please use a meaningful parameter name for the method.
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_r364027356
##
File path: helix-core/src/main/java/org/apache/helix/model/CloudConfig.java
##
@@ -0,0 +1,290 @@
+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.HelixException;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.cloud.constants.CloudProvider;
+
+/**
+ * Cloud configurations
+ */
+public class CloudConfig extends HelixProperty {
+ /**
+ * Configurable characteristics of a cloud.
+ * NOTE: Do NOT use this field name directly, use its corresponding
getter/setter in the
+ * CloudConfig.
+ */
+ public enum CloudConfigProperty {
+CLOUD_ENABLED, // determine whether the cluster is inside cloud
environment.
+CLOUD_PROVIDER, // the environment the cluster is in, e.g. Azure, AWS, or
Customized
+CLOUD_ID, // the cloud Id that belongs to this cluster.
+
+// If user uses Helix supported default provider, the below entries will
not be shown in
+// CloudConfig.
+CLOUD_INFO_SOURCE, // the source for retrieving the cloud information.
+CLOUD_INFO_PROCESSOR_NAME // the name of the function that processes the
fetching and parsing of
+ // cloud information.
+ }
+
+ /* Default values */
+ private static final boolean DEFAULT_CLOUD_ENABLED = false;
+
+ /**
+ * Instantiate the CloudConfig for the cloud
+ * @param cluster
+ */
+ public CloudConfig(String cluster) {
+super(cluster);
+ }
+
+ /**
+ * Instantiate with a pre-populated record
+ * @param record a ZNRecord corresponding to a cloud configuration
+ */
+ public CloudConfig(ZNRecord record) {
+super(record);
+ }
+
+ /**
+ * Instantiate the config using each field individually.
+ * Users should use CloudConfig.Builder to create CloudConfig.
+ * @param cluster
+ * @param enabled
+ * @param cloudID
+ */
+ public CloudConfig(String cluster, boolean enabled, CloudProvider
cloudProvider, String cloudID,
+ List cloudInfoSource, String cloudProcessorName) {
+super(cluster);
+_record.setBooleanField(CloudConfigProperty.CLOUD_ENABLED.name(), enabled);
+_record.setSimpleField(CloudConfigProperty.CLOUD_PROVIDER.name(),
cloudProvider.name());
+_record.setSimpleField(CloudConfigProperty.CLOUD_ID.name(), cloudID);
+if (cloudProvider.equals(CloudProvider.CUSTOMIZED)) {
+ _record
+
.setSimpleField(CloudConfigProperty.CLOUD_INFO_PROCESSOR_NAME.name(),
cloudProcessorName);
+ _record.setListField(CloudConfigProperty.CLOUD_INFO_SOURCE.name(),
cloudInfoSource);
+}
+ }
+
+ /**
+ * Enable/Disable the CLOUD_ENABLED field.
+ * @param enabled
+ */
+ public void setCloudEnabled(boolean enabled) {
+_record.setBooleanField(CloudConfigProperty.CLOUD_ENABLED.name(), enabled);
+ }
+
+ /**
+ * Whether CLOUD_ENABLED field is enabled or not.
+ * @return
+ */
+ public boolean isCloudEnabled() {
+return _record.getBooleanField(CloudConfigProperty.CLOUD_ENABLED.name(),
false);
+ }
+
+ /**
+ * Set the cloudID field.
+ * @param cloudID
+ */
+ public void setCloudID(String cloudID) {
+_record.setSimpleField(CloudConfigProperty.CLOUD_ID.name(), cloudID);
+ }
+
+ /**
+ * Get the CloudID field.
+ * @return CloudID
+ */
+ public String getCloudID() {
+return _record.getSimpleField(CloudConfigProperty.CLOUD_ID.name());
+ }
+
+ /**
+ * Set the CLOUD_INFO_SOURCE field.
+ * @param cloudInfoSources
+ */
+ public void setCloudInfoSource(List cloudInfoSources) {
+_record.setListField(CloudConfigProperty.CLOUD_INFO_SOURCE.name(),
cloudInfoSources);
+ }
+
+ /**
+ * Get the CLOUD_INFO_SOURCE field.
+ * @return CLOUD_INFO_SOURCE field.
+ */
+ public List getCloudInfoSources() {
+return _record.getListField(CloudConfigProperty.CLOUD_INFO_SOURCE.name());
+ }
+
+ /**
+ * Set the CLOUD_INFO_PROCESSOR_NAME field.
+ * @param cloudInfoProcessorName
+ */
+ public void
[GitHub] [helix] zhangmeng916 commented on a change in pull request #673: Add Helix Distributed lock module
zhangmeng916 commented on a change in pull request #673: Add Helix Distributed
lock module
URL: https://github.com/apache/helix/pull/673#discussion_r364025165
##
File path: helix-lock/pom.xml
##
@@ -0,0 +1,162 @@
+
+
+http://maven.apache.org/POM/4.0.0;
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+
+org.apache.helix
+helix
+0.9.2-SNAPSHOT
+
+ 4.0.0
+
+ helix-lock
+ bundle
+ Apache Helix :: Distributed Lock
+
+
+
+ org.apache.helix*,
+ org.slf4j*;version="[1.6,2)",
+ *
+
+
org.apache.helix.lock*;version="${project.version};-noimport:=true
+
+
+
+
+ org.yaml
+ snakeyaml
+ 1.17
+
+
Review comment:
yeah, removed.
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364008883
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/zookeeper/ZkClient.java
##
@@ -703,8 +939,6 @@ public void process(WatchedEvent event) {
if (event.getState() == KeeperState.Expired) {
getEventLock().getZNodeEventCondition().signalAll();
getEventLock().getDataChangedCondition().signalAll();
- // We also have to notify all listeners that something might have
changed
- fireAllEvents();
Review comment:
It is in the finally block. With this change, when getShutdownTrigger() ==
true, some notifications might be missed.
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 #673: Add Helix Distributed lock module
zhangmeng916 commented on a change in pull request #673: Add Helix Distributed
lock module
URL: https://github.com/apache/helix/pull/673#discussion_r364018917
##
File path: helix-lock/pom.xml
##
@@ -0,0 +1,162 @@
+
+
+http://maven.apache.org/POM/4.0.0;
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+
+org.apache.helix
+helix
+0.9.2-SNAPSHOT
+
+ 4.0.0
+
+ helix-lock
+ bundle
+ Apache Helix :: Distributed Lock
+
+
+
+ org.apache.helix*,
+ org.slf4j*;version="[1.6,2)",
+ *
+
+
org.apache.helix.lock*;version="${project.version};-noimport:=true
+
+
+
+
+ org.yaml
+ snakeyaml
+ 1.17
+
+
Review comment:
I actually don't find this one in parent pom. Do you have another example
that is in the parent?
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364004887
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -1134,9 +1159,31 @@ public void handleNewSession(final String sessionId)
throws Exception {
// Wait until we get a non-zero session id. Otherwise, getSessionId()
might be null.
waitUntilConnected();
-// TODO: filter out stale sessions here.
+/*
+ * Filter out stale sessions. If a session id is not null and not the same
as current session
+ * id, this session is expired. With this filtering, expired sessions are
NOT handled,
+ * so performance is expected to improve.
+ */
+if (sessionId != null && !getSessionId().equals(sessionId)) {
+ LOG.info("Session is expired and not handled. Expected: {}. Actual:
{}.", sessionId,
+ getSessionId());
+ return;
+}
+
+/*
+ * When a null session id is passed in, we will take current session's id
for following
+ * operations. Please note that current session might not be the one we
expect to handle,
+ * because the one we expect might be already expired when the zk event is
waiting in the
+ * event queue. Why we use current session here is for backward
compatibility with the old
+ * method handleNewSession().
+ */
+if (sessionId == null) {
+ sessionId = getSessionId();
+ LOG.warn("Session id: is passed in. Current session id: {} will
be used.", sessionId);
Review comment:
nit, LOG.debug?
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364003612
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -697,10 +699,19 @@ void createClient() throws Exception {
int retryCount = 0;
while (retryCount < 3) {
try {
-// TODO: synchronize this block and wait for the new non-zero session
ID updated.
-_zkclient.waitUntilConnected(_connectionInitTimeout,
TimeUnit.MILLISECONDS);
+// TODO: get session id from waitUntilConnected to avoid race condition
Review comment:
This is OK. Just a reminder that we need to finish this TODO until the bug
is finished.
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364003612
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -697,10 +699,19 @@ void createClient() throws Exception {
int retryCount = 0;
while (retryCount < 3) {
try {
-// TODO: synchronize this block and wait for the new non-zero session
ID updated.
-_zkclient.waitUntilConnected(_connectionInitTimeout,
TimeUnit.MILLISECONDS);
+// TODO: get session id from waitUntilConnected to avoid race condition
Review comment:
This is OK. Just a reminder that we need to finish this TODO until the bug
is fixed.
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r364001145
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -178,89 +194,18 @@ private void createLiveInstance() {
}
}
-boolean retry;
-do {
- retry = false;
- try {
-_zkclient.createEphemeral(liveInstancePath, liveInstance.getRecord());
-LOG.info("LiveInstance created, path: " + liveInstancePath + ",
sessionId: " + liveInstance.getEphemeralOwner());
- } catch (ZkNodeExistsException e) {
Review comment:
Or we can keep the wait and retry logic.
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r363999846
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -178,89 +194,18 @@ private void createLiveInstance() {
}
}
-boolean retry;
-do {
- retry = false;
- try {
-_zkclient.createEphemeral(liveInstancePath, liveInstance.getRecord());
-LOG.info("LiveInstance created, path: " + liveInstancePath + ",
sessionId: " + liveInstance.getEphemeralOwner());
- } catch (ZkNodeExistsException e) {
Review comment:
Although it should be impossible that we see this exception anymore, shall
we still catch it and throw HelixException?
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 #642: Fix handleNewSession creating ephemeral node with expired session
jiajunwang commented on a change in pull request #642: Fix handleNewSession
creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r363994859
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##
@@ -97,10 +98,25 @@ public ParticipantManager(HelixManager manager,
HelixZkClient zkclient, int sess
}
/**
- * Handle new session for a participang.
- * @throws Exception
+ * Handles a new session for a participant. The new session's id is passed
in when participant
+ * manager is created, as it is required in
+ * {@link
org.apache.helix.manager.zk.zookeeper.ZkClient#createEphemeral(String, Object,
String)}
Review comment:
The explanation is good here, but this link to the code is not necessary.
And maybe hard for us to maintain.
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 #673: Add Helix Distributed lock module
pkuwm commented on a change in pull request #673: Add Helix Distributed lock
module
URL: https://github.com/apache/helix/pull/673#discussion_r363986291
##
File path: helix-lock/pom.xml
##
@@ -0,0 +1,162 @@
+
+
+http://maven.apache.org/POM/4.0.0;
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance;
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd;>
+
+org.apache.helix
+helix
+0.9.2-SNAPSHOT
+
+ 4.0.0
+
+ helix-lock
+ bundle
+ Apache Helix :: Distributed Lock
+
+
+
+ org.apache.helix*,
+ org.slf4j*;version="[1.6,2)",
+ *
+
+
org.apache.helix.lock*;version="${project.version};-noimport:=true
+
+
+
+
+ org.yaml
+ snakeyaml
+ 1.17
+
+
Review comment:
These dependencies are the same in the parent pom. I think it would be more
clear and easier to maintain if you remove these dependencies in this child
pom, because this child pom file will inherit all properties and dependencies
from parent POM. Unless you want to use a different version/additional
dependency.
Same with following duplicate dependencies.
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 #673: Add Helix Distributed lock module
zhangmeng916 commented on a change in pull request #673: Add Helix Distributed lock module URL: https://github.com/apache/helix/pull/673#discussion_r363972815 ## File path: helix-lock/pom.xml ## @@ -0,0 +1,189 @@ + Review comment: I removed some that won't be needed for 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] zhangmeng916 commented on a change in pull request #673: Add Helix Distributed lock module
zhangmeng916 commented on a change in pull request #673: Add Helix Distributed lock module URL: https://github.com/apache/helix/pull/673#discussion_r363972602 ## File path: helix-lock/helix-lock-0.9.2-SNAPSHOT.ivy ## @@ -0,0 +1,54 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Review comment: I removed some that won't be needed for 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] dasahcc commented on a change in pull request #673: Add Helix Distributed lock module
dasahcc commented on a change in pull request #673: Add Helix Distributed lock module URL: https://github.com/apache/helix/pull/673#discussion_r363903946 ## File path: helix-lock/pom.xml ## @@ -0,0 +1,189 @@ + Review comment: Same for this file. Remove all the components, which are unnecessary. Otherwise, the file is too large and imports are not required. 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 issue #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped.
pkuwm commented on issue #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped. URL: https://github.com/apache/helix/pull/670#issuecomment-571727613 LGTM. +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] dasahcc commented on a change in pull request #673: Add Helix Distributed lock module
dasahcc commented on a change in pull request #673: Add Helix Distributed lock module URL: https://github.com/apache/helix/pull/673#discussion_r363903646 ## File path: helix-lock/helix-lock-0.9.2-SNAPSHOT.ivy ## @@ -0,0 +1,54 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Review comment: Do you need all these? 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_r363901582
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/CloudAccessor.java
##
@@ -0,0 +1,152 @@
+package org.apache.helix.rest.server.resources.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.IOException;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Response;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKUtil;
+import org.apache.helix.manager.zk.client.HelixZkClient;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Path("/clusters")
+public class CloudAccessor extends AbstractHelixResource {
+ private static Logger _logger =
LoggerFactory.getLogger(CloudAccessor.class.getName());
+
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+}
+
+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 (Exception ex) {
Review comment:
Let's have another catch for HelixException to differentiate it with other
general error. It could be validation exception.
Same as following places.
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_r363902937
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/CloudAccessor.java
##
@@ -0,0 +1,152 @@
+package org.apache.helix.rest.server.resources.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.IOException;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Response;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKUtil;
+import org.apache.helix.manager.zk.client.HelixZkClient;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Path("/clusters")
+public class CloudAccessor extends AbstractHelixResource {
+ private static Logger _logger =
LoggerFactory.getLogger(CloudAccessor.class.getName());
+
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+}
+
+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 (Exception ex) {
+ _logger.error("Error in adding a CloudConfig to cluster: " + clusterId,
ex);
+ return badRequest(ex.getMessage());
+}
+
+return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/cloudconfig")
+ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+}
+
+ConfigAccessor configAccessor = new ConfigAccessor(zkClient);
+CloudConfig cloudConfig = configAccessor.getCloudConfig(clusterId);
+
+if (cloudConfig != null) {
+ return JSONRepresentation(cloudConfig.getRecord());
+}
+
+return notFound();
+ }
+
+ @POST
+ @Path("{clusterId}/cloudconfig")
+ public Response updateCloudConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+}
+
+// Here to update cloud config
+Command command;
+if (commandStr == null || commandStr.isEmpty()) {
+ command = Command.update; // Default behavior
+} else {
+ try {
+command = getCommand(commandStr);
+ } catch (HelixException ex) {
+return badRequest(ex.getMessage());
+ }
+}
+
+HelixAdmin admin = getHelixAdmin();
+try {
+ switch (command) {
+case delete: {
+ admin.removeCloudConfig(clusterId);
+}
+break;
+case update: {
+ ZNRecord record;
Review comment:
move record read before command switch.
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:
[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_r363902396
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/CloudAccessor.java
##
@@ -0,0 +1,152 @@
+package org.apache.helix.rest.server.resources.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.IOException;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Response;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKUtil;
+import org.apache.helix.manager.zk.client.HelixZkClient;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Path("/clusters")
+public class CloudAccessor extends AbstractHelixResource {
+ private static Logger _logger =
LoggerFactory.getLogger(CloudAccessor.class.getName());
+
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
Review comment:
This is not a good practice here. User may get confused with 404 and without
any other information here. Let's have clear message that cluster is not
properly setup.
Same as following places.
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_r363897194
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/CloudAccessor.java
##
@@ -0,0 +1,152 @@
+package org.apache.helix.rest.server.resources.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.IOException;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Response;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKUtil;
+import org.apache.helix.manager.zk.client.HelixZkClient;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Path("/clusters")
+public class CloudAccessor extends AbstractHelixResource {
+ private static Logger _logger =
LoggerFactory.getLogger(CloudAccessor.class.getName());
+
+ @PUT
+ @Path("{clusterId}/cloudconfig")
+ public Response addCloudConfig(@PathParam("clusterId") String clusterId,
String content) {
+
+HelixZkClient zkClient = getHelixZkClient();
+if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+}
+
+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!");
+}
Review comment:
Let's make it as a general function in AbstractResourceAccessor since it has
been widely used in the code.
Same as following places
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 merged pull request #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped.
jiajunwang merged pull request #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped. URL: https://github.com/apache/helix/pull/670 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_r363896739
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##
@@ -132,18 +133,42 @@ public Response getClusterInfo(@PathParam("clusterId")
String clusterId) {
@PUT
@Path("{clusterId}")
public Response createCluster(@PathParam("clusterId") String clusterId,
- @DefaultValue("false") @QueryParam("recreate") String recreate) {
+ @DefaultValue("false") @QueryParam("recreate") String recreate,
+ @DefaultValue("false") @QueryParam("addCloudConfig") String
addCloudConfig,
+ String content) {
+
boolean recreateIfExists = Boolean.valueOf(recreate);
+boolean cloudConfigIncluded = Boolean.valueOf(addCloudConfig);
+
+
ClusterSetup clusterSetup = getClusterSetup();
-try {
- clusterSetup.addCluster(clusterId, recreateIfExists);
-} catch (Exception ex) {
- _logger.error("Failed to create cluster " + clusterId + ", exception: "
+ ex);
- return serverError(ex);
-}
+if (!cloudConfigIncluded) {
Review comment:
Let's reorganize the code. There are duplicated pieces.
You can:
1. Define CloudConfig object
2. Create this object if enabled and move this piece of code to a separate
function. Otherwise, keep it as null.
3. Call the addCluster with CloudConfig object.
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_r363887244
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/CloudAccessor.java
##
@@ -0,0 +1,152 @@
+package org.apache.helix.rest.server.resources.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.IOException;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Response;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZKUtil;
+import org.apache.helix.manager.zk.client.HelixZkClient;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Path("/clusters")
Review comment:
Does this work? It conflict with ClusterAccessor. It would be better to
merge the logic into ClusterAccessor.
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 #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped.
jiajunwang commented on issue #670: Reorgnize the test case so the new WAGED expand cluster tests are not skipped. URL: https://github.com/apache/helix/pull/670#issuecomment-571726398 This PR is ready to be merged, approved by @alirezazamani 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 opened a new pull request #675: Add REST API for Cluster Creation with CloudConfig
alirezazamani opened a new pull request #675: Add REST API for Cluster Creation
with CloudConfig
URL: https://github.com/apache/helix/pull/675
### Issues
- [x] My PR addresses the following Helix issues and references them in the
PR title:
Fixes #674
### Description
- [x] Here are some details about my PR, including screenshots of any UI
changes:
This PR contains the relevant REST API calls for cluster creation.
This change allows user to create cluster with CloudConfig.
Multiple tests have been added in order to check the functionality of REST
calls.
### Tests
- [x] The following is the result of the "mvn test" command on the
appropriate module:
New tests have been added.
TestCloudAccessor.testAddCloudConfigNonExistedCluster
TestCloudAccessor.testAddCloudConfig
TestCloudAccessor.testDeleteCloudConfig
TestCloudAccessor.testUpdateCloudConfig
TestClusterAccessor.testAddClusterWithCloudConfig
TestClusterAccessor.testAddClusterWithInvalidCloudConfig
TestClusterAccessor.testAddClusterWithInvalidCustomizedCloudConfig
TestClusterAccessor.testAddClusterWithValidCustomizedCloudConfig
TestClusterAccessor.testAddClusterWithCloudConfigDisabledCloud
Test Result 1: helix-core: mvn test
[INFO] Tests run: 904, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
3,444.069 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 904, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] BUILD SUCCESS
[INFO]
[INFO] Total time: 57:29 min
[INFO] Finished at: 2020-01-06T17:05:55-08:00
[INFO]
Test Result 2: helix-rest: mvn test
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
26.832 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] BUILD SUCCESS
[INFO]
[INFO] Total time: 32.481 s
[INFO] Finished at: 2020-01-07T09:25:20-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
