You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by jl...@apache.org on 2019/10/01 17:10:36 UTC
[incubator-pinot] branch master updated: Refactor
toggleInstanceState API (#4560)
This is an automated email from the ASF dual-hosted git repository.
jlli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 8a80760 Refactor toggleInstanceState API (#4560)
8a80760 is described below
commit 8a80760a20483b878510d39a31e020400c5ee06c
Author: Jialiang Li <jl...@linkedin.com>
AuthorDate: Tue Oct 1 10:10:30 2019 -0700
Refactor toggleInstanceState API (#4560)
* Fix toggle instance API
* Fix ControllerInstanceToggleTest
---
.../resources/PinotInstanceRestletResource.java | 12 ++--
.../helix/core/PinotHelixResourceManager.java | 79 ++++++++++++++--------
.../helix/ControllerInstanceToggleTest.java | 26 +++++--
3 files changed, 78 insertions(+), 39 deletions(-)
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
index 79788c1..e6c0553 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
@@ -130,13 +130,17 @@ public class PinotInstanceRestletResource {
}
if (StateType.ENABLE.name().equalsIgnoreCase(state)) {
- if (!pinotHelixResourceManager.enableInstance(instanceName).isSuccessful()) {
- throw new ControllerApplicationException(LOGGER, "Failed to enable instance " + instanceName,
+ PinotResourceManagerResponse response = pinotHelixResourceManager.enableInstance(instanceName);
+ if (!response.isSuccessful()) {
+ throw new ControllerApplicationException(LOGGER,
+ "Failed to enable instance " + instanceName + " - " + response.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR);
}
} else if (StateType.DISABLE.name().equalsIgnoreCase(state)) {
- if (!pinotHelixResourceManager.disableInstance(instanceName).isSuccessful()) {
- throw new ControllerApplicationException(LOGGER, "Failed to disable instance " + instanceName,
+ PinotResourceManagerResponse response = pinotHelixResourceManager.disableInstance(instanceName);
+ if (!response.isSuccessful()) {
+ throw new ControllerApplicationException(LOGGER,
+ "Failed to disable instance " + instanceName + " - " + response.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR);
}
} else if (StateType.DROP.name().equalsIgnoreCase(state)) {
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
index 9afd643..81ebe7f 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
@@ -2084,11 +2084,11 @@ public class PinotHelixResourceManager {
}
public PinotResourceManagerResponse enableInstance(String instanceName) {
- return toggleInstance(instanceName, true, 10);
+ return enableInstance(instanceName, true, 10_000L);
}
public PinotResourceManagerResponse disableInstance(String instanceName) {
- return toggleInstance(instanceName, false, 10);
+ return enableInstance(instanceName, false, 10_000L);
}
/**
@@ -2140,52 +2140,71 @@ public class PinotHelixResourceManager {
* Keeps checking until ideal-state is successfully updated or times out.
*
* @param instanceName: Name of Instance for which the status needs to be toggled.
- * @param toggle: 'True' for ONLINE 'False' for OFFLINE.
- * @param timeOutInSeconds: Time-out for setting ideal-state.
+ * @param enableInstance: 'True' for enabling the instance and 'False' for disabling the instance.
+ * @param timeOutMs: Time-out for setting ideal-state.
* @return
*/
- public PinotResourceManagerResponse toggleInstance(String instanceName, boolean toggle, int timeOutInSeconds) {
+ private PinotResourceManagerResponse enableInstance(String instanceName, boolean enableInstance, long timeOutMs) {
if (!instanceExists(instanceName)) {
return PinotResourceManagerResponse.failure("Instance " + instanceName + " not found");
}
- _helixAdmin.enableInstance(_helixClusterName, instanceName, toggle);
- long deadline = System.currentTimeMillis() + 1000 * timeOutInSeconds;
- boolean toggleSucceed = false;
- String beforeToggleStates =
- (toggle) ? SegmentOnlineOfflineStateModel.OFFLINE : SegmentOnlineOfflineStateModel.ONLINE;
+ _helixAdmin.enableInstance(_helixClusterName, instanceName, enableInstance);
+ long intervalWaitTimeMs = 500L;
+ long deadline = System.currentTimeMillis() + timeOutMs;
+ String offlineState = SegmentOnlineOfflineStateModel.OFFLINE;
while (System.currentTimeMillis() < deadline) {
- toggleSucceed = true;
PropertyKey liveInstanceKey = _keyBuilder.liveInstance(instanceName);
LiveInstance liveInstance = _helixDataAccessor.getProperty(liveInstanceKey);
if (liveInstance == null) {
- return toggle ? PinotResourceManagerResponse.FAILURE : PinotResourceManagerResponse.SUCCESS;
- }
- PropertyKey instanceCurrentStatesKey = _keyBuilder.currentStates(instanceName, liveInstance.getSessionId());
- List<CurrentState> instanceCurrentStates = _helixDataAccessor.getChildValues(instanceCurrentStatesKey);
- if (instanceCurrentStates == null) {
- return PinotResourceManagerResponse.SUCCESS;
+ if (!enableInstance) {
+ // If we disable the instance, we actually don't care whether live instance being null. Thus, returning success should be good.
+ // Otherwise, wait until timeout.
+ return PinotResourceManagerResponse.SUCCESS;
+ }
} else {
- for (CurrentState currentState : instanceCurrentStates) {
- for (String state : currentState.getPartitionStateMap().values()) {
- if (beforeToggleStates.equals(state)) {
- toggleSucceed = false;
+ boolean toggleSucceeded = true;
+ // Checks all the current states fall into the target states
+ PropertyKey instanceCurrentStatesKey = _keyBuilder.currentStates(instanceName, liveInstance.getSessionId());
+ List<CurrentState> instanceCurrentStates = _helixDataAccessor.getChildValues(instanceCurrentStatesKey);
+ if (instanceCurrentStates.isEmpty()) {
+ return PinotResourceManagerResponse.SUCCESS;
+ } else {
+ for (CurrentState currentState : instanceCurrentStates) {
+ for (String state : currentState.getPartitionStateMap().values()) {
+ // If instance is enabled, all the partitions should not eventually be offline.
+ // If instance is disabled, all the partitions should eventually be offline.
+ // TODO: Handle the case when realtime segments are in OFFLINE state because there're some problem with realtime segment consumption,
+ // and realtime segment will mark itself as OFFLINE in ideal state.
+ // Issue: https://github.com/apache/incubator-pinot/issues/4653
+ if ((enableInstance && !offlineState.equals(state)) || (!enableInstance && offlineState.equals(state))) {
+ toggleSucceeded = false;
+ break;
+ }
+ }
+ if (!toggleSucceeded) {
+ break;
}
}
}
- }
- if (toggleSucceed) {
- return (toggle) ? PinotResourceManagerResponse.success("Instance " + instanceName + " enabled")
- : PinotResourceManagerResponse.success("Instance " + instanceName + " disabled");
- } else {
- try {
- Thread.sleep(500);
- } catch (InterruptedException e) {
+ if (toggleSucceeded) {
+ return (enableInstance) ? PinotResourceManagerResponse.success("Instance " + instanceName + " enabled")
+ : PinotResourceManagerResponse.success("Instance " + instanceName + " disabled");
}
}
+
+ try {
+ Thread.sleep(intervalWaitTimeMs);
+ } catch (InterruptedException e) {
+ LOGGER.warn("Got interrupted when sleeping for {}ms to wait until the current state matched for instance: {}",
+ intervalWaitTimeMs, instanceName);
+ return PinotResourceManagerResponse
+ .failure("Got interrupted when waiting for instance to be " + (enableInstance ? "enabled" : "disabled"));
+ }
}
- return PinotResourceManagerResponse.failure("Instance enable/disable failed, timeout");
+ return PinotResourceManagerResponse
+ .failure("Instance " + (enableInstance ? "enable" : "disable") + " failed, timeout");
}
public RebalanceResult rebalanceTable(String tableNameWithType, Configuration rebalanceConfig)
diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerInstanceToggleTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerInstanceToggleTest.java
index da923c6..2ccf9fd 100644
--- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerInstanceToggleTest.java
+++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerInstanceToggleTest.java
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.controller.helix;
+import java.io.IOException;
import java.util.Set;
import org.apache.helix.model.ExternalView;
import org.apache.pinot.common.config.TableConfig;
@@ -26,6 +27,7 @@ import org.apache.pinot.common.config.TagNameUtils;
import org.apache.pinot.common.utils.CommonConstants;
import org.apache.pinot.common.utils.helix.HelixHelper;
import org.apache.pinot.controller.utils.SegmentMetadataMockUtils;
+import org.apache.pinot.util.TestUtils;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
@@ -34,6 +36,7 @@ import org.testng.annotations.Test;
public class ControllerInstanceToggleTest extends ControllerTest {
private static final String RAW_TABLE_NAME = "testTable";
+ private static final long TIMEOUT_MS = 10_000L;
private static final String OFFLINE_TABLE_NAME = TableNameBuilder.OFFLINE.tableNameWithType(RAW_TABLE_NAME);
private static final String SERVER_TAG_NAME = TagNameUtils.getOfflineTagForTenant(null);
private static final String BROKER_TAG_NAME = TagNameUtils.getBrokerTagForTenant(null);
@@ -75,25 +78,25 @@ public class ControllerInstanceToggleTest extends ControllerTest {
// Disable server instances
int numEnabledInstances = NUM_INSTANCES;
for (String instanceName : _helixAdmin.getInstancesInClusterWithTag(getHelixClusterName(), SERVER_TAG_NAME)) {
- sendPostRequest(_controllerRequestURLBuilder.forInstanceState(instanceName), "disable");
+ toggleInstanceState(instanceName, "disable");
checkNumOnlineInstancesFromExternalView(OFFLINE_TABLE_NAME, --numEnabledInstances);
}
// Enable server instances
for (String instanceName : _helixAdmin.getInstancesInClusterWithTag(getHelixClusterName(), SERVER_TAG_NAME)) {
- sendPostRequest(_controllerRequestURLBuilder.forInstanceState(instanceName), "ENABLE");
+ toggleInstanceState(instanceName, "ENABLE");
checkNumOnlineInstancesFromExternalView(OFFLINE_TABLE_NAME, ++numEnabledInstances);
}
// Disable broker instances
for (String instanceName : _helixAdmin.getInstancesInClusterWithTag(getHelixClusterName(), BROKER_TAG_NAME)) {
- sendPostRequest(_controllerRequestURLBuilder.forInstanceState(instanceName), "Disable");
+ toggleInstanceState(instanceName, "Disable");
checkNumOnlineInstancesFromExternalView(CommonConstants.Helix.BROKER_RESOURCE_INSTANCE, --numEnabledInstances);
}
// Enable broker instances
for (String instanceName : _helixAdmin.getInstancesInClusterWithTag(getHelixClusterName(), BROKER_TAG_NAME)) {
- sendPostRequest(_controllerRequestURLBuilder.forInstanceState(instanceName), "Enable");
+ toggleInstanceState(instanceName, "Enable");
checkNumOnlineInstancesFromExternalView(CommonConstants.Helix.BROKER_RESOURCE_INSTANCE, ++numEnabledInstances);
}
@@ -104,9 +107,22 @@ public class ControllerInstanceToggleTest extends ControllerTest {
.getPartitionSet().size(), 0);
}
+ private void toggleInstanceState(String instanceName, String state) {
+ // It may take time for an instance to toggle the state.
+ TestUtils.waitForCondition(aVoid -> {
+ try {
+ sendPostRequest(_controllerRequestURLBuilder.forInstanceState(instanceName), state);
+ } catch (IOException ioe) {
+ // receive non-200 status code
+ return false;
+ }
+ return true;
+ }, TIMEOUT_MS, "Failed to toggle instance state: '" + state + "' for instance: " + instanceName);
+ }
+
private void checkNumOnlineInstancesFromExternalView(String resourceName, int expectedNumOnlineInstances)
throws InterruptedException {
- long endTime = System.currentTimeMillis() + 10_000L;
+ long endTime = System.currentTimeMillis() + TIMEOUT_MS;
while (System.currentTimeMillis() < endTime) {
ExternalView resourceExternalView = _helixAdmin.getResourceExternalView(getHelixClusterName(), resourceName);
Set<String> instanceSet = HelixHelper.getOnlineInstanceFromExternalView(resourceExternalView);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org