[GitHub] [helix] narendly merged pull request #611: Add REST API endpoints for WAGED Rebalancer
narendly merged pull request #611: Add REST API endpoints for WAGED Rebalancer URL: https://github.com/apache/helix/pull/611 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 #611: Add REST API endpoints for WAGED Rebalancer
narendly commented on a change in pull request #611: Add REST API endpoints for
WAGED Rebalancer
URL: https://github.com/apache/helix/pull/611#discussion_r350560438
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##
@@ -240,7 +240,15 @@ public Response updateCluster(@PathParam("clusterId")
String clusterId,
helixAdmin.manuallyEnableMaintenanceMode(clusterId, command ==
Command.enableMaintenanceMode,
content, customFieldsMap);
break;
-
+case enableWagedRebalanceForAllResources:
Review comment:
Why would it be required?
That sounds more appropriate for the user manual though.
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 #611: Add REST API endpoints for WAGED Rebalancer
jiajunwang commented on a change in pull request #611: Add REST API endpoints
for WAGED Rebalancer
URL: https://github.com/apache/helix/pull/611#discussion_r350558676
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##
@@ -240,7 +240,15 @@ public Response updateCluster(@PathParam("clusterId")
String clusterId,
helixAdmin.manuallyEnableMaintenanceMode(clusterId, command ==
Command.enableMaintenanceMode,
content, customFieldsMap);
break;
-
+case enableWagedRebalanceForAllResources:
Review comment:
Question, we will require users to enable maintenance mode before calling
this cmd, right?
Shall we comment here or just in the doc? Either one works for me.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 issue #516: Implement the propertyStore read endpoint
i3wangyi commented on issue #516: Implement the propertyStore read endpoint URL: https://github.com/apache/helix/pull/516#issuecomment-558423038 latest mvn test under helix-rest folder, [INFO] Tests run: 90, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 31.416 s - in TestSuite [INFO] [INFO] Results: [INFO] [INFO] Tests run: 90, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 38.012 s [INFO] Finished at: 2019-11-25T17:55:22-08:00 [INFO] Will rebase from master and resolve conflicts 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] i3wangyi commented on a change in pull request #516: Implement the propertyStore read endpoint
i3wangyi commented on a change in pull request #516: Implement the
propertyStore read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350505964
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,129 @@
+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.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+private final String _path;
+
+public PropertyStoreSerializer(String path) {
+ _path = path;
+}
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ if (bytes == null || bytes.length == 0) {
+throw new ZkMarshallingError("Data unavailable from path: " + _path);
+ }
+ // first, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(_path);
+znRecord.setSimpleField(_path, new String(bytes));
+return znRecord;
+ }
+ return content;
+}
+
+// The hashCode and equals methods' implementations ensure all instances
are the same
+@Override
Review comment:
no longer valid.
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 #516: Implement the propertyStore read endpoint
i3wangyi commented on a change in pull request #516: Implement the
propertyStore read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350506030
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java
##
@@ -117,9 +121,20 @@ public HelixDataAccessor getDataAccssor(String
clusterName) {
}
}
+ public ZkBaseDataAccessor getZkBaseDataAccessor(ZkSerializer
serializer) {
+synchronized (_zkBaseDataAccessorBySerializer) {
+ if (!_zkBaseDataAccessorBySerializer.containsKey(serializer)) {
+ZkBaseDataAccessor baseDataAccessor = new
ZkBaseDataAccessor<>(_zkAddr, serializer);
+_zkBaseDataAccessorBySerializer.put(serializer, baseDataAccessor);
+ }
+}
+return _zkBaseDataAccessorBySerializer.get(serializer);
Review comment:
no longer valid
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 #516: Implement the propertyStore read endpoint
i3wangyi commented on a change in pull request #516: Implement the
propertyStore read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350505658
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
+znRecord.setSimpleField(DEFAULT_KEY, new String(bytes));
+return znRecord;
+ }
+ return content;
+}
+ }
+
+ /**
+ * Sample HTTP URLs:
+ * http:///clusters/{clusterId}/propertyStore/
+ * It refers to the /PROPERTYSTORE/ in Helix metadata store
+ * @param clusterId The cluster Id
+ * @param path path parameter is like "abc/abc/abc" in the URL
+ * @return JSON object as the response
+ */
+ @GET
+ @Path("{path: .+}")
+ public Response getPropertyByPath(@PathParam("clusterId") String clusterId,
Review comment:
Yes, client only needs to pass the subpath after `PROPERTYSTORE` and you can
check the line `final String recordPath =
PropertyPathBuilder.propertyStore(clusterId) + path;`.
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 #516: Implement the propertyStore read endpoint
i3wangyi commented on a change in pull request #516: Implement the
propertyStore read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350505428
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
Review comment:
It's @narendly's idea, many clients already assume the content to be
ZnRecord in their client-side library; I'm fine either one
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #516: Implement the propertyStore read endpoint
i3wangyi commented on a change in pull request #516: Implement the
propertyStore read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350505239
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
+znRecord.setSimpleField(DEFAULT_KEY, new String(bytes));
+return znRecord;
+ }
+ return content;
+}
+ }
+
+ /**
+ * Sample HTTP URLs:
+ * http:///clusters/{clusterId}/propertyStore/
+ * It refers to the /PROPERTYSTORE/ in Helix metadata store
+ * @param clusterId The cluster Id
+ * @param path path parameter is like "abc/abc/abc" in the URL
+ * @return JSON object as the response
+ */
+ @GET
+ @Path("{path: .+}")
+ public Response getPropertyByPath(@PathParam("clusterId") String clusterId,
+ @PathParam("path") String path) {
+path = "/" + path;
+if (!isPathValid(path)) {
+ LOG.info("The propertyStore path {} is invalid for cluster {}", path,
clusterId);
+ throw new
WebApplicationException(Response.status(Response.Status.BAD_REQUEST).entity(
Review comment:
thanks! updated, the other NOT_FOUND response should be a valid case
(badRequest() returns http code 400)
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350500034
##
File path: helix-core/src/test/java/org/apache/helix/integration/TestDrop.java
##
@@ -55,16 +55,26 @@
* @param participants
*/
private void assertEmptyCSandEV(String clusterName, String db,
- MockParticipantManager[] participants) {
+ MockParticipantManager[] participants) throws Exception {
HelixDataAccessor accessor =
new ZKHelixDataAccessor(clusterName, new
ZkBaseDataAccessor<>(_gZkClient));
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
-Assert.assertNull(accessor.getProperty(keyBuilder.externalView(db)));
+boolean isExternalViewNull = TestHelper.verify(() -> {
+ ExternalView externalView =
accessor.getProperty(keyBuilder.externalView(db));
+ return (externalView == null);
+}, TestHelper.WAIT_DURATION);
+Assert.assertTrue(isExternalViewNull);
for (MockParticipantManager participant : participants) {
String instanceName = participant.getInstanceName();
String sessionId = participant.getSessionId();
-
Assert.assertNull(accessor.getProperty(keyBuilder.currentState(instanceName,
sessionId, db)));
+ boolean isCurrentStateNull = TestHelper.verify(() -> {
+CurrentState currentState =
accessor.getProperty(keyBuilder.currentState(instanceName, sessionId, db));
+return (currentState == null);
+ }, TestHelper.WAIT_DURATION);
+ Assert.assertTrue(isCurrentStateNull);
+
+
Review comment:
Redundant space?
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350500093
##
File path:
helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestWorkflowControllerDataProvider.java
##
@@ -30,21 +30,26 @@
public class TestWorkflowControllerDataProvider extends TaskTestBase {
@Test
- public void testResourceConfigRefresh() throws InterruptedException {
+ public void testResourceConfigRefresh() throws Exception {
Workflow.Builder builder = new Workflow.Builder("TEST");
JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG);
builder.addJob(WorkflowGenerator.JOB_NAME_1, jobBuilder);
_driver.start(builder.build());
-Thread.sleep(4000);
+
WorkflowControllerDataProvider cache =
new WorkflowControllerDataProvider("CLUSTER_" +
TestHelper.getTestClassName());
-cache.requireFullRefresh();
-cache.refresh(_manager.getHelixDataAccessor());
-Assert.assertEquals(cache.getJobConfigMap().size(), 1);
-Assert.assertEquals(cache.getWorkflowConfigMap().size(), 1);
-Assert.assertEquals(cache.getContexts().size(), 2);
+
+boolean expectedValuesAchieved = TestHelper.verify(() -> {
+ cache.requireFullRefresh();
+ cache.refresh(_manager.getHelixDataAccessor());
+ int configMapSize = cache.getJobConfigMap().size();
+ int workflowConfigMapSize = cache.getWorkflowConfigMap().size();
+ int contextsSize = cache.getContexts().size();
+ return (configMapSize == 1 && workflowConfigMapSize == 1 && contextsSize
== 2);
+}, TestHelper.WAIT_DURATION);
+Assert.assertTrue(expectedValuesAchieved);
Review comment:
Redundant space?
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350499858
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -29,10 +29,14 @@
import org.testng.Assert;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
+import org.apache.helix.TestHelper;
public class TestGetLastScheduledTaskExecInfo extends TaskTestBase {
private final static String TASK_START_TIME_KEY = "START_TIME";
private final static long INVALID_TIMESTAMP = -1L;
+ private static int SHORT_EXECUTION_TIME = 10;
+ private static int LONG_EXECUTION_TIME = ;
+ private static final int DELETE_DELAY = 30 * 1000;
Review comment:
nit: make them longs with L appended at the end :)
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350499925
##
File path:
helix-core/src/test/java/org/apache/helix/tools/TestClusterVerifier.java
##
@@ -132,8 +132,14 @@ public void testDisablePartitionAndStopInstance() throws
InterruptedException {
// Disable partition for 1 instance, then Full-Auto ExternalView should
not match IdealState.
_admin.enablePartition(false, _clusterName,
_participants[0].getInstanceName(), FULL_AUTO_RESOURCES[0],
Lists.newArrayList(FULL_AUTO_RESOURCES[0] + "_0"));
-Thread.sleep(1000);
-Assert.assertFalse(strictMatchVerifier.verify(3000));
+
+boolean isVerifiedFalse = TestHelper.verify(() -> {
+ HelixClusterVerifier strictMatchVerifierTemp =
+ new
StrictMatchExternalViewVerifier.Builder(_clusterName).setZkClient(_gZkClient).build();
+ boolean verified = strictMatchVerifierTemp.verify(3000);
+ return (!verified);
+}, 60 * 1000);
Review comment:
Magic number?
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 issue #624: Stabilize 5 unstable tests
alirezazamani commented on issue #624: Stabilize 5 unstable tests URL: https://github.com/apache/helix/pull/624#issuecomment-558398687 Result of mvn test after the addressing the comments: [INFO] Tests run: 886, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,262.438 s - in TestSuite [INFO] [INFO] Results: [INFO] [INFO] Tests run: 886, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 54:27 min [INFO] Finished at: 2019-11-25T16:08:16-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] alirezazamani edited a comment on issue #624: Stabilize 5 unstable tests
alirezazamani edited a comment on issue #624: Stabilize 5 unstable tests URL: https://github.com/apache/helix/pull/624#issuecomment-558398687 Result of mvn test after addressing the comments: [INFO] Tests run: 886, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,262.438 s - in TestSuite [INFO] [INFO] Results: [INFO] [INFO] Tests run: 886, Failures: 0, Errors: 0, Skipped: 0 [INFO] [INFO] [INFO] BUILD SUCCESS [INFO] [INFO] Total time: 54:27 min [INFO] Finished at: 2019-11-25T16:08:16-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] alirezazamani commented on issue #624: Stabilize 5 unstable tests
alirezazamani commented on issue #624: Stabilize 5 unstable tests URL: https://github.com/apache/helix/pull/624#issuecomment-558390660 @dasahcc I did put some general information about the fixes in the PR description. Here are more detail: 1- TestWorkflowControllerDataProvider: In this test, the Tread.sleep(4000) has been used followed by multiple Assert statements to check values such as cache.getJobConfigMap().size() and cache.getWorkflowConfigMap().size(). However, Thread.sleep() is not desirable since there might be fast or slow controller threads that reach the expected values sooner/later than expected. Hence here it is preferable to used TestHelper.verify to check read these values and check them in a loop. 2- TestDrop: In this test Thread.sleep has been used followed by Assert which is eliminated in this PR and has been substituted with TestHelper.verify. 3- TestGetLastScheduledTaskExecInfo: This test were rely on Thread.sleep(). This has been changed to TestHelper.verify. In this test, one second delay has been used to make sure controller has enough time to schedule the tasks. However, this one second is not enough in most of the cases. Hence, in this PR, instead of Thread.sleep, it is better and more robust to use TestHelper.verify and count the number of scheduled task by checking their start time. If number of tasks with start time reaches the expected number of scheduled tasks, it means the test can continue to the next steps. 4- TestStopWorkflow: In this test Thread.sleep has been used to make sure all of the tasks are running. Instead of Thread.sleep, I used pollForJobState to make sure jobs are running and I also got all the tasks status in the context and checked if the tasks have reached the running state. Hence, we can be sure that the queue are in desired state after these checks. Also, previously, a global variable has been use among all the tasks to stop the tasks. Having a global variable to stop all the task might cause race condition between the tasks (concurrent get and set with different order). This has been changed to local variable. 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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350461885
##
File path:
helix-core/src/test/java/org/apache/helix/tools/TestClusterVerifier.java
##
@@ -132,8 +132,14 @@ public void testDisablePartitionAndStopInstance() throws
InterruptedException {
// Disable partition for 1 instance, then Full-Auto ExternalView should
not match IdealState.
_admin.enablePartition(false, _clusterName,
_participants[0].getInstanceName(), FULL_AUTO_RESOURCES[0],
Lists.newArrayList(FULL_AUTO_RESOURCES[0] + "_0"));
-Thread.sleep(1000);
-Assert.assertFalse(strictMatchVerifier.verify(3000));
+
Review comment:
Thanks for the comment. I removed the Thread.sleep and change it with
BestPossibleExternalViewVerifier to make sure resources have converged. As we
discussed, I also add BestPossibleExternalViewVerifier next to
StrictMatchExternalViewVerifier to make sure test functionality has not been
changed. 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] alirezazamani commented on a change in pull request #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350460009
##
File path:
helix-core/src/test/java/org/apache/helix/tools/TestClusterVerifier.java
##
@@ -173,15 +179,15 @@ public void testResourceSubset() throws
InterruptedException {
// Ensure that this passes even when one resource is down
_admin.enableInstance(_clusterName, "localhost_12918", false);
-Thread.sleep(1000);
-_admin.enableCluster(_clusterName, false);
-_admin.enableInstance(_clusterName, "localhost_12918", true);
ZkHelixClusterVerifier verifier =
new
BestPossibleExternalViewVerifier.Builder(_clusterName).setZkClient(_gZkClient)
.setResources(Sets.newHashSet(testDB)).build();
Assert.assertTrue(verifier.verifyByPolling());
+_admin.enableCluster(_clusterName, false);
Review comment:
Fixed. Thanks. Now those checks are together.
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 #624: Stabilize 5 unstable tests
jiajunwang commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350454502
##
File path:
helix-core/src/test/java/org/apache/helix/tools/TestClusterVerifier.java
##
@@ -132,8 +132,14 @@ public void testDisablePartitionAndStopInstance() throws
InterruptedException {
// Disable partition for 1 instance, then Full-Auto ExternalView should
not match IdealState.
_admin.enablePartition(false, _clusterName,
_participants[0].getInstanceName(), FULL_AUTO_RESOURCES[0],
Lists.newArrayList(FULL_AUTO_RESOURCES[0] + "_0"));
-Thread.sleep(1000);
-Assert.assertFalse(strictMatchVerifier.verify(3000));
+
Review comment:
Please refer to my change in the wagedRebalancer branch, using the
bestPossibleVerifier directly should help to solve the problem.
Although I'm not sure if that is also related to what I have changed, could
you please try that one?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #624: Stabilize 5 unstable tests
jiajunwang commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350455688
##
File path:
helix-core/src/test/java/org/apache/helix/tools/TestClusterVerifier.java
##
@@ -173,15 +179,15 @@ public void testResourceSubset() throws
InterruptedException {
// Ensure that this passes even when one resource is down
_admin.enableInstance(_clusterName, "localhost_12918", false);
-Thread.sleep(1000);
-_admin.enableCluster(_clusterName, false);
-_admin.enableInstance(_clusterName, "localhost_12918", true);
ZkHelixClusterVerifier verifier =
new
BestPossibleExternalViewVerifier.Builder(_clusterName).setZkClient(_gZkClient)
.setResources(Sets.newHashSet(testDB)).build();
Assert.assertTrue(verifier.verifyByPolling());
+_admin.enableCluster(_clusterName, false);
Review comment:
This changes the purpose of this test.
The BestPossibleExternalViewVerifier and StrictMatchExternalViewVerifier
should be tested with exactly the same condition.
Why do we need to move these 2 changes to make
BestPossibleExternalViewVerifier verifying pass?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350451308
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
Review comment:
Addressed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350451193
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestStopWorkflow.java
##
@@ -203,13 +202,20 @@ public void testResumeTaskForQuota() throws
InterruptedException {
_driver.start(workflowBuilder_1.build());
-Thread.sleep(2000L); // Sleep until each task really is in progress
+// Check the jobs are in progress. Each job has one task.
+for (int i = 0; i < 30; i++) {
+ _driver.pollForJobState(workflowName_1, workflowName_1 + "_JOB" + i,
TaskState.IN_PROGRESS);
+}
Review comment:
Yes. It can also be in INIT. I added the check got the tasks.
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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450988
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
-Assert.assertEquals(startTimesFastTasks.get(startTimesFastTasks.size() -
1),
-lastScheduledTaskTs);
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_3_job_0");
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.COMPLETED);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Start the new queue with new task configuration.
+// Start new queue that has one job with 4 short tasks and record start
time of the tasks
+List startTimesFastTasks = setupTasks(queueName, 4, 10, 4);
+// Wait till the job is in progress
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
+hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.COMPLETED
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesFastTasks.get(startTimesFastTasks.size() -
1).equals(lastScheduledTaskTs));
+}, 30 * 1000);
Review comment:
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,
[GitHub] [helix] alirezazamani commented on a change in pull request #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450890
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
-Assert.assertEquals(startTimesFastTasks.get(startTimesFastTasks.size() -
1),
-lastScheduledTaskTs);
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_3_job_0");
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.COMPLETED);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Start the new queue with new task configuration.
+// Start new queue that has one job with 4 short tasks and record start
time of the tasks
+List startTimesFastTasks = setupTasks(queueName, 4, 10, 4);
Review comment:
Fixed. Thanks for the comment.
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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450890
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
-Assert.assertEquals(startTimesFastTasks.get(startTimesFastTasks.size() -
1),
-lastScheduledTaskTs);
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_3_job_0");
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.COMPLETED);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Start the new queue with new task configuration.
+// Start new queue that has one job with 4 short tasks and record start
time of the tasks
+List startTimesFastTasks = setupTasks(queueName, 4, 10, 4);
Review comment:
Fixed. Thanks for the commend.
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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450601
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
Review comment:
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] alirezazamani commented on a change in pull request #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450421
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestStopWorkflow.java
##
@@ -275,13 +281,16 @@ private void cleanupParticipants(int numNodes) {
* A mock task class that models a short-lived task to be stopped.
*/
private class StopTask extends MockTask {
+boolean toStop = false;
Review comment:
Fixed. I changed it to _stopFlag.
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 #624: Stabilize 5 unstable tests
alirezazamani commented on a change in pull request #624: Stabilize 5 unstable
tests
URL: https://github.com/apache/helix/pull/624#discussion_r350450233
##
File path:
helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestWorkflowControllerDataProvider.java
##
@@ -30,21 +30,26 @@
public class TestWorkflowControllerDataProvider extends TaskTestBase {
@Test
- public void testResourceConfigRefresh() throws InterruptedException {
+ public void testResourceConfigRefresh() throws Exception {
Workflow.Builder builder = new Workflow.Builder("TEST");
JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG);
builder.addJob(WorkflowGenerator.JOB_NAME_1, jobBuilder);
_driver.start(builder.build());
-Thread.sleep(4000);
+
WorkflowControllerDataProvider cache =
new WorkflowControllerDataProvider("CLUSTER_" +
TestHelper.getTestClassName());
-cache.requireFullRefresh();
-cache.refresh(_manager.getHelixDataAccessor());
-Assert.assertEquals(cache.getJobConfigMap().size(), 1);
-Assert.assertEquals(cache.getWorkflowConfigMap().size(), 1);
-Assert.assertEquals(cache.getContexts().size(), 2);
+
+boolean expectedValuesAchieved = TestHelper.verify(() -> {
+ cache.requireFullRefresh();
+ cache.refresh(_manager.getHelixDataAccessor());
+ int configMapSize = cache.getJobConfigMap().size();
+ int workflowConfigMapSize = cache.getWorkflowConfigMap().size();
+ int contextsSize = cache.getContexts().size();
+ return (configMapSize == 1 && workflowConfigMapSize == 1 && contextsSize
== 2);
+}, 30 * 1000);
Review comment:
Good comment. Fixed. We already have TestHelper.WAIT_DURATION. I leveraged
that 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] zhangmeng916 removed a comment on issue #600: add Helix cloud interface and implementation skeleton methods
zhangmeng916 removed a comment on issue #600: add Helix cloud interface and implementation skeleton methods URL: https://github.com/apache/helix/pull/600#issuecomment-557224337 This PR is ready to be merged, approved by @dasahcc final commit message: Add Helix cloud interface and implementation skeleton 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] alirezazamani removed a comment on issue #589: Add CloudConfig operations
alirezazamani removed a comment on issue #589: Add CloudConfig operations URL: https://github.com/apache/helix/pull/589#issuecomment-558350725 This PR is ready to be merged, approved by @dasahcc. Title: Add CloudConfig Related Code Body: In order to move toward supporting cloud environments and auto-registration, we need to have a CloudConfig Znode under the Config. In this commit, the code regarding the CloudConfig is added. 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 issue #516: Implement the propertyStore read endpoint
dasahcc commented on issue #516: Implement the propertyStore read endpoint URL: https://github.com/apache/helix/pull/516#issuecomment-558350340 > > > > > Why this PR contains customized serializer change? I would suggest to separate the PR. 1 for customized serializer change and another for propertyStore REST. > > > > > > > > > > > > The property store accessor just re-uses the existing features by the custom serializer and I think it's needed for parsing the content under PropertyStore. The PR is all about property store read api only. > > > > > > > > > I am confused about that. For propertyStore, it uses ZNRecord. Default serializer should be fine. Why it depends on the customized serializer. > > > Even we need that, we should separate them out. Have a separate customized serializer first. Let's review that. After that, let's talk about the propertyStore support. Dont merge them together. > > > > > > Does the property store always use ZNrecord? My understanding of the scope is to provide generic rest endpoints for users to read/write property store content no matter what format it is. > > As I said, even if we need the part of customized serializer logic, let's have one PR for customized serializer. After it has been checkedin, let's have the implementation of PropertyStore. Was confused with another PR for customized serialize. Synced up offline. This change should be self-contained. 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 issue #589: Add CloudConfig operations
alirezazamani commented on issue #589: Add CloudConfig operations URL: https://github.com/apache/helix/pull/589#issuecomment-558350725 This PR is ready to be merged, approved by @dasahcc. Title: Add CloudConfig Related Code Body: In order to move toward supporting cloud environments and auto-registration, we need to have a CloudConfig Znode under the Config. In this commit, the code regarding the CloudConfig is added. 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 #516: Implement the propertyStore read endpoint
dasahcc commented on a change in pull request #516: Implement the propertyStore
read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350433017
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
+znRecord.setSimpleField(DEFAULT_KEY, new String(bytes));
+return znRecord;
+ }
+ return content;
+}
+ }
+
+ /**
+ * Sample HTTP URLs:
+ * http:///clusters/{clusterId}/propertyStore/
+ * It refers to the /PROPERTYSTORE/ in Helix metadata store
+ * @param clusterId The cluster Id
+ * @param path path parameter is like "abc/abc/abc" in the URL
+ * @return JSON object as the response
+ */
+ @GET
+ @Path("{path: .+}")
+ public Response getPropertyByPath(@PathParam("clusterId") String clusterId,
+ @PathParam("path") String path) {
+path = "/" + path;
+if (!isPathValid(path)) {
+ LOG.info("The propertyStore path {} is invalid for cluster {}", path,
clusterId);
+ throw new
WebApplicationException(Response.status(Response.Status.BAD_REQUEST).entity(
Review comment:
We do have badRequest() response function.
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 #516: Implement the propertyStore read endpoint
dasahcc commented on a change in pull request #516: Implement the propertyStore
read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350433638
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
+znRecord.setSimpleField(DEFAULT_KEY, new String(bytes));
+return znRecord;
+ }
+ return content;
+}
+ }
+
+ /**
+ * Sample HTTP URLs:
+ * http:///clusters/{clusterId}/propertyStore/
+ * It refers to the /PROPERTYSTORE/ in Helix metadata store
+ * @param clusterId The cluster Id
+ * @param path path parameter is like "abc/abc/abc" in the URL
+ * @return JSON object as the response
+ */
+ @GET
+ @Path("{path: .+}")
+ public Response getPropertyByPath(@PathParam("clusterId") String clusterId,
Review comment:
I think we should limit the path to under PROPERTYSTORE folder. @narendly
This is what we agreed with new API change, right?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #516: Implement the propertyStore read endpoint
dasahcc commented on a change in pull request #516: Implement the propertyStore
read endpoint
URL: https://github.com/apache/helix/pull/516#discussion_r350432594
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PropertyStoreAccessor.java
##
@@ -0,0 +1,112 @@
+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 javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.I0Itec.zkclient.exception.ZkMarshallingError;
+import org.I0Itec.zkclient.serialize.ZkSerializer;
+import org.apache.helix.AccessOption;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+@Path("/clusters/{clusterId}/propertyStore")
+public class PropertyStoreAccessor extends AbstractHelixResource {
+ private static Logger LOG =
LoggerFactory.getLogger(PropertyStoreAccessor.class);
+
+ public static class PropertyStoreSerializer implements ZkSerializer {
+private static final String DEFAULT_KEY = "default";
+private static final ZNRecordSerializer ZN_RECORD_SERIALIZER = new
ZNRecordSerializer();
+
+// used for writing the serialized content to property store path
+@Override
+public byte[] serialize(Object o)
+throws ZkMarshallingError {
+ return (byte[]) o;
+}
+
+// used for reading the raw content of property store path to ZnRecord
format
+@Override
+public Object deserialize(byte[] bytes)
+throws ZkMarshallingError {
+ // firstly, try to deserialize the bytearray into ZnRecord using {@link
ZNRecordSerializer}
+ ZNRecord content = (ZNRecord) ZN_RECORD_SERIALIZER.deserialize(bytes);
+ // if first trial fails, fallback to return a simple/default znRecord
+ if (content == null) {
+ZNRecord znRecord = new ZNRecord(DEFAULT_KEY);
Review comment:
Why we need create another ZNRecord? If it is failed to deserialize by
ZNRecord, we can just simply return a json response instead of a complete
ZNRecord with empty list fields and map fields.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #589: Add CloudConfig operations
alirezazamani commented on a change in pull request #589: Add CloudConfig
operations
URL: https://github.com/apache/helix/pull/589#discussion_r350432158
##
File path: helix-core/src/main/java/org/apache/helix/PropertyKey.java
##
@@ -214,6 +215,16 @@ public PropertyKey clusterConfig() {
_clusterName, ConfigScopeProperty.CLUSTER.toString(), _clusterName);
}
+
+/**
+ * Get a property key associated with this Cloud configuration
+ * @return {@link PropertyKey}
+ */
+public PropertyKey cloudConfig() {
+ return new PropertyKey(CONFIGS, ConfigScopeProperty.CLOUD,
CloudConfig.class,
+ _clusterName, ConfigScopeProperty.CLOUD.toString(), _clusterName);
Review comment:
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] alirezazamani commented on a change in pull request #589: Add CloudConfig operations
alirezazamani commented on a change in pull request #589: Add CloudConfig
operations
URL: https://github.com/apache/helix/pull/589#discussion_r350432211
##
File path: helix-core/src/main/java/org/apache/helix/model/CloudConfig.java
##
@@ -0,0 +1,253 @@
+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;
+
+
+/**
+ * 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_ID, // the cloud Id that belongs to this cluster.
+CLOUD_INFO_URLS, // the URLs from where to retrieve the cloud information.
+CLOUD_INFO_PARSE_CLASS_NAME, // the name of the function that parses the
topology.
+ }
+
+ /* Default values */
+ public 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.
+ * @param cluster
+ * @param enabled
+ * @param cloudID
+ * @param cloudInfoURLs
+ * @param cloudInfoParserClassName
+ */
+ public CloudConfig(String cluster, boolean enabled, String cloudID,
List cloudInfoURLs,
+ String cloudInfoParserClassName) {
+super(cluster);
+_record.setBooleanField(CloudConfigProperty.CLOUD_ENABLED.name(), enabled);
+_record.setSimpleField(CloudConfigProperty.CLOUD_ID.name(), cloudID);
+_record.setListField(CloudConfigProperty.CLOUD_INFO_URLS.name(),
cloudInfoURLs);
+
_record.setSimpleField(CloudConfigProperty.CLOUD_INFO_PARSE_CLASS_NAME.name(),
+cloudInfoParserClassName);
+ }
+
+ /**
+ * 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_URLS field.
+ * @param cloudInfoURLs
+ */
+ public void setCloudInfoURLs(List cloudInfoURLs) {
+_record.setListField(CloudConfigProperty.CLOUD_INFO_URLS.name(),
cloudInfoURLs);
+ }
+
+ /**
+ * Get the CLOUD_INFO_URLS field.
+ * @return CLOUD_INFO_URLS field.
+ */
+ public List getCloudInfoURLs() {
+return _record.getListField(CloudConfigProperty.CLOUD_INFO_URLS.name());
+ }
+
+ /**
+ * Set the CLOUD_INFO_PARSE_CLASS_NAME field.
+ * @param cloudInfoParserClassName
+ */
+ public void setCloudInfoParserClassName(String cloudInfoParserClassName) {
+
_record.setSimpleField(CloudConfigProperty.CLOUD_INFO_PARSE_CLASS_NAME.name(),
+cloudInfoParserClassName);
+ }
+
+ /**
+ * Get the CLOUD_INFO_PARSE_CLASS_NAME field.
+ * @return CLOUD_INFO_PARSE_CLASS_NAME field.
+ */
+ public String getCloudInfoParserClassName() {
+return
_record.getSimpleField(CloudConfigProperty.CLOUD_INFO_PARSE_CLASS_NAME.name());
+ }
+
+ public static class Builder {
+private String _clusterName = null;
+private boolean
[GitHub] [helix] jiajunwang commented on a change in pull request #611: Add REST API endpoints for WAGED Rebalancer
jiajunwang commented on a change in pull request #611: Add REST API endpoints for WAGED Rebalancer URL: https://github.com/apache/helix/pull/611#discussion_r350374583 ## File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java ## @@ -66,7 +66,14 @@ rebalance, reset, resetPartitions, -removeInstanceTag +removeInstanceTag, +addResource, +addWagedResource, +getResource, +validateWeight, +enableWagedRebalance, Review comment: As we discussed, we need one more API to enable the WAGED rebalancer for all resources. 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 issue #516: Implement the propertyStore read endpoint
dasahcc commented on issue #516: Implement the propertyStore read endpoint URL: https://github.com/apache/helix/pull/516#issuecomment-558271790 > > > > Why this PR contains customized serializer change? I would suggest to separate the PR. 1 for customized serializer change and another for propertyStore REST. > > > > > > > > > The property store accessor just re-uses the existing features by the custom serializer and I think it's needed for parsing the content under PropertyStore. The PR is all about property store read api only. > > > > > > I am confused about that. For propertyStore, it uses ZNRecord. Default serializer should be fine. Why it depends on the customized serializer. > > Even we need that, we should separate them out. Have a separate customized serializer first. Let's review that. After that, let's talk about the propertyStore support. Dont merge them together. > > Does the property store always use ZNrecord? My understanding of the scope is to provide generic rest endpoints for users to read/write property store content no matter what format it is. As I said, even if we need the part of customized serializer logic, let's have one PR for customized serializer. After it has been checkedin, let's have the implementation of PropertyStore. 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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350325364
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
Review comment:
Do you think you could make recurring 30 * 1000's a constant?
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350323625
##
File path:
helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestWorkflowControllerDataProvider.java
##
@@ -30,21 +30,26 @@
public class TestWorkflowControllerDataProvider extends TaskTestBase {
@Test
- public void testResourceConfigRefresh() throws InterruptedException {
+ public void testResourceConfigRefresh() throws Exception {
Workflow.Builder builder = new Workflow.Builder("TEST");
JobConfig.Builder jobBuilder =
JobConfig.Builder.fromMap(WorkflowGenerator.DEFAULT_JOB_CONFIG);
builder.addJob(WorkflowGenerator.JOB_NAME_1, jobBuilder);
_driver.start(builder.build());
-Thread.sleep(4000);
+
WorkflowControllerDataProvider cache =
new WorkflowControllerDataProvider("CLUSTER_" +
TestHelper.getTestClassName());
-cache.requireFullRefresh();
-cache.refresh(_manager.getHelixDataAccessor());
-Assert.assertEquals(cache.getJobConfigMap().size(), 1);
-Assert.assertEquals(cache.getWorkflowConfigMap().size(), 1);
-Assert.assertEquals(cache.getContexts().size(), 2);
+
+boolean expectedValuesAchieved = TestHelper.verify(() -> {
+ cache.requireFullRefresh();
+ cache.refresh(_manager.getHelixDataAccessor());
+ int configMapSize = cache.getJobConfigMap().size();
+ int workflowConfigMapSize = cache.getWorkflowConfigMap().size();
+ int contextsSize = cache.getContexts().size();
+ return (configMapSize == 1 && workflowConfigMapSize == 1 && contextsSize
== 2);
+}, 30 * 1000);
Review comment:
Is there any significance to the wait times (30 * 1000 or 10 * 1000's)? If
not, could we make it a public static constant in TestHelper and use that
throughout these tests?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350325013
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
Review comment:
Nit: could we make a constant and name it something more
appropriate?
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350325575
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
-Assert.assertEquals(startTimesFastTasks.get(startTimesFastTasks.size() -
1),
-lastScheduledTaskTs);
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_3_job_0");
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.COMPLETED);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Start the new queue with new task configuration.
+// Start new queue that has one job with 4 short tasks and record start
time of the tasks
+List startTimesFastTasks = setupTasks(queueName, 4, 10, 4);
+// Wait till the job is in progress
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
+hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.COMPLETED
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesFastTasks.get(startTimesFastTasks.size() -
1).equals(lastScheduledTaskTs));
+}, 30 * 1000);
Review comment:
magic number
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,
[GitHub] [helix] narendly commented on a change in pull request #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350324383
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestStopWorkflow.java
##
@@ -275,13 +281,16 @@ private void cleanupParticipants(int numNodes) {
* A mock task class that models a short-lived task to be stopped.
*/
private class StopTask extends MockTask {
+boolean toStop = false;
Review comment:
What does "toStop" mean? Is there a better name 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] narendly commented on a change in pull request #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350325918
##
File path:
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
##
@@ -41,37 +42,47 @@ public void beforeClass() throws Exception {
}
@Test
- public void testGetLastScheduledTaskExecInfo() throws InterruptedException {
-List startTimesWithStuckTasks = setupTasks("TestWorkflow_2", 5,
);
+ public void testGetLastScheduledTaskExecInfo() throws Exception {
+// Start new queue that has one job with long tasks and record start time
of the tasks
+String queueName = TestHelper.getTestMethodName();
+List startTimesWithStuckTasks = setupTasks(queueName, 5, ,
2);
// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_2", "TestWorkflow_2_job_0",
TaskState.IN_PROGRESS);
+_driver.pollForJobState(queueName, queueName + "_job_0",
TaskState.IN_PROGRESS);
// First two must be -1 (two tasks are stuck), and API call must return
the last value (most
// recent timestamp)
Assert.assertEquals(startTimesWithStuckTasks.get(0).longValue(),
INVALID_TIMESTAMP);
Assert.assertEquals(startTimesWithStuckTasks.get(1).longValue(),
INVALID_TIMESTAMP);
-Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_2");
-// Get the last timestamp (which will be most recent)
-Assert.assertEquals(startTimesWithStuckTasks.get(4), lastScheduledTaskTs);
-TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo("TestWorkflow_2");
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_2_job_0");
-// Workflow 2 will be stuck, so its partition state will be RUNNING
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.RUNNING);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Workflow will be stuck so its partition state will be Running
+boolean hasQueueReachedDesiredState = TestHelper.verify(() -> {
+ Long lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp(queueName);
+ TaskExecutionInfo execInfo =
_driver.getLastScheduledTaskExecutionInfo(queueName);
+ return (execInfo.getJobName().equals(queueName + "_job_0")
+ && execInfo.getTaskPartitionState() == TaskPartitionState.RUNNING
+ && execInfo.getStartTimeStamp().equals(lastScheduledTaskTs)
+ && startTimesWithStuckTasks.get(4).equals(lastScheduledTaskTs));
+}, 30 * 1000);
+Assert.assertTrue(hasQueueReachedDesiredState);
-List startTimesFastTasks = setupTasks("TestWorkflow_3", 4, 10);
-// Wait till the job is in progress
-_driver.pollForJobState("TestWorkflow_3", "TestWorkflow_3_job_0",
TaskState.IN_PROGRESS);
-// API call needs to return the most recent timestamp (value at last index)
-lastScheduledTaskTs =
_driver.getLastScheduledTaskTimestamp("TestWorkflow_3");
-execInfo = _driver.getLastScheduledTaskExecutionInfo("TestWorkflow_3");
+// Stop and delete the queue
+_driver.stop(queueName);
+_driver.deleteAndWaitForCompletion(queueName, 30 * 1000);
-Assert.assertEquals(startTimesFastTasks.get(startTimesFastTasks.size() -
1),
-lastScheduledTaskTs);
-Assert.assertEquals(execInfo.getJobName(), "TestWorkflow_3_job_0");
-Assert.assertEquals(execInfo.getTaskPartitionState(),
TaskPartitionState.COMPLETED);
-Assert.assertEquals(execInfo.getStartTimeStamp(), lastScheduledTaskTs);
+// Start the new queue with new task configuration.
+// Start new queue that has one job with 4 short tasks and record start
time of the tasks
+List startTimesFastTasks = setupTasks(queueName, 4, 10, 4);
Review comment:
Explain what these numbers mean.
10 -> make it a constant?
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 #624: Stabilize 5 unstable tests
narendly commented on a change in pull request #624: Stabilize 5 unstable tests
URL: https://github.com/apache/helix/pull/624#discussion_r350324191
##
File path:
helix-core/src/test/java/org/apache/helix/integration/task/TestStopWorkflow.java
##
@@ -203,13 +202,20 @@ public void testResumeTaskForQuota() throws
InterruptedException {
_driver.start(workflowBuilder_1.build());
-Thread.sleep(2000L); // Sleep until each task really is in progress
+// Check the jobs are in progress. Each job has one task.
+for (int i = 0; i < 30; i++) {
+ _driver.pollForJobState(workflowName_1, workflowName_1 + "_JOB" + i,
TaskState.IN_PROGRESS);
+}
Review comment:
Nit: Could a job be in progress but its task not in RUNINNG state? Do you
need to check for the task state?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
