jiajunwang commented on a change in pull request #1007:
URL: https://github.com/apache/helix/pull/1007#discussion_r425562832
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
+ if (!_zkClient.exists(instancePath)) {
+ errMessage = String.format(
+ "Can't reset state for %s.%s on %s, because %s has never existed"
+ " in cluster %s",
Review comment:
Why there is still a "+", it is not necessary, right?
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
+ if (!_zkClient.exists(instancePath)) {
+ errMessage = String.format(
+ "Can't reset state for %s.%s on %s, because %s has never existed"
+ " in cluster %s",
Review comment:
BTW, if this is for the format, just use the auto format supported by
the IDE after you import the Helix code style.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
+ if (!_zkClient.exists(instancePath)) {
+ errMessage = String.format(
+ "Can't reset state for %s.%s on %s, because %s has never existed"
+ " in cluster %s",
+ resourceName, partitionNames, instanceName, instanceName,
clusterName);
+ } else {
+ errMessage = String.format("Can't reset state for %s.%s on %s, because
%s does not live in"
+ + " cluster %s anymore", resourceName, partitionNames,
instanceName, instanceName,
+ clusterName);
+ }
+ throw new HelixException(errMessage);
}
// check resource group exists
IdealState idealState =
accessor.getProperty(keyBuilder.idealStates(resourceName));
if (idealState == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + resourceName + " is not added");
+ throw new HelixException(String
+ .format("Can't reset state for %s.%s on %s, because %s is not
added", resourceName,
+ partitionNames, instanceName, resourceName));
}
// check partition exists in resource group
Set<String> resetPartitionNames = new HashSet<String>(partitionNames);
+ Set<String> partitions;
if (idealState.getRebalanceMode() == RebalanceMode.CUSTOMIZED) {
- Set<String> partitions = new
HashSet<String>(idealState.getRecord().getMapFields().keySet());
- if (!partitions.containsAll(resetPartitionNames)) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because not all " + partitionNames + " exist");
- }
+ partitions = idealState.getRecord().getMapFields().keySet();
} else {
- Set<String> partitions = new
HashSet<String>(idealState.getRecord().getListFields().keySet());
- if (!partitions.containsAll(resetPartitionNames)) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because not all " + partitionNames + " exist");
- }
+ partitions = idealState.getRecord().getListFields().keySet();
+ }
+ if (!partitions.containsAll(resetPartitionNames)) {
+ throw new HelixException(String
+ .format("Can't reset state for %s.%s on %s, because not all %s
exist", resourceName,
Review comment:
It looks like there are many error messages look like this, I think we
can have a private method to generate the formated messaged based on the input,
the parameters could be: partitionNames, instanceName, failure reason (so the
callers just have different logic of generating the different reasons).
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
+ if (!_zkClient.exists(instancePath)) {
+ errMessage = String.format(
+ "Can't reset state for %s.%s on %s, because %s has never existed"
+ " in cluster %s",
+ resourceName, partitionNames, instanceName, instanceName,
clusterName);
+ } else {
+ errMessage = String.format("Can't reset state for %s.%s on %s, because
%s does not live in"
+ + " cluster %s anymore", resourceName, partitionNames,
instanceName, instanceName,
+ clusterName);
+ }
+ throw new HelixException(errMessage);
}
// check resource group exists
IdealState idealState =
accessor.getProperty(keyBuilder.idealStates(resourceName));
if (idealState == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + resourceName + " is not added");
+ throw new HelixException(String
+ .format("Can't reset state for %s.%s on %s, because %s is not
added", resourceName,
+ partitionNames, instanceName, resourceName));
}
// check partition exists in resource group
Set<String> resetPartitionNames = new HashSet<String>(partitionNames);
+ Set<String> partitions;
Review comment:
nit, you could use (idealState.getRebalanceMode() ==
RebalanceMode.CUSTOMIZED)? idealState.getRecord().getMapFields().keySet() :
partitions = idealState.getRecord().getListFields().keySet() to simplify it a
little bit.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
+ if (!_zkClient.exists(instancePath)) {
+ errMessage = String.format(
+ "Can't reset state for %s.%s on %s, because %s has never existed"
+ " in cluster %s",
+ resourceName, partitionNames, instanceName, instanceName,
clusterName);
+ } else {
+ errMessage = String.format("Can't reset state for %s.%s on %s, because
%s does not live in"
+ + " cluster %s anymore", resourceName, partitionNames,
instanceName, instanceName,
Review comment:
The check only shows it is not alive for now. "anymore" is not accurate,
we can remove it.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,35 +573,41 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check the instance is alive
LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has never exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName,
instanceName);
+ String errMessage;
Review comment:
nit, You can init the errMessage like String.format( "Can't reset state
for %s.%s on %s,", resourceName, partitionNames, instanceName), then in the
different branches, append more content.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
