You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/09 18:10:20 UTC
[GitHub] [helix] jiajunwang opened a new pull request #878: Get the
MinActiveReplica from Resource Config and Idea State
jiajunwang opened a new pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
#877
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Read the MinActiveReplica config from both Resource Config and Idea State.
Currently, the same configuration item can be configured in both Resource Config and Ideal State. In theory, the Resource Config is the right place.
This is the first step to migrate the IdealState usage to read the Resource Config.
### Tests
- [ ] The following tests are written for this issue:
TBD
- [ ] The following is the result of the "mvn test" command on the appropriate module:
TBD
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #878: Get the
MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #878: Get
the MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r389921866
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,89 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ // Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ // 1. The fields shall have exactly the same meaning.
+ // 2. The value shall be fully compatible, no additional calculation involved.
+ // 3. Resource Config items have a high priority.
+
+ // Return a newly constructed resource config to avoid the original value be modified.
+ ResourceConfig mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name(),
+ idealState.getInstanceGroupTag());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name(),
+ idealState.getMaxPartitionsPerInstance());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.NUM_PARTITIONS.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfigProperty.NUM_PARTITIONS.name(), idealState.getNumPartitions());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name(),
+ idealState.getStateModelDefRef());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name(),
+ idealState.getStateModelFactoryName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name(),
+ idealState.getReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name())) {
+ mergedZNRecord.setIntField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name(),
+ idealState.getMinActiveReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name())) {
+ mergedZNRecord.setBooleanField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name(),
+ idealState.isEnabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name(),
+ idealState.getResourceGroupName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name(),
+ idealState.getResourceType());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name(),
+ idealState.isExternalViewDisabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name(),
+ idealState.isDelayRebalanceEnabled());
+ }
Review comment:
I see a lot of duplicate code here -
could we just create an array of all `ResourceConfig.ResourceConfigProperty`s and just copy the 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #878: Get the
MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r390630659
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,72 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ *
+ * Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ * 1. The fields shall have exactly the same meaning.
+ * 2. The value shall be fully compatible, no additional calculation involved.
+ * 3. Resource Config items have a high priority.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ if (idealState == null) {
+ return resourceConfig;
+ }
+ ResourceConfig mergedResourceConfig;
+ if (resourceConfig != null) {
+ if (!resourceConfig.getResourceName().equals(idealState.getResourceName())) {
+ throw new IllegalArgumentException(String.format(
+ "Cannot merge the IdealState of resource %s with the ResourceConfig of resource %s",
+ resourceConfig.getResourceName(), idealState.getResourceName()));
+ }
+ // Copy the resource config to avoid the original value being modified unexpectedly.
+ mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ } else {
+ // If no resource config specified, construct one based on the Idealstate.
+ mergedResourceConfig = new ResourceConfig(idealState.getResourceName());
+ }
+ // Fill the compatible Idealstate fields to the ResourceConfig if possible.
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
Review comment:
Can we make a list of constants for these necessary field and use a loop to do the merge instead of have these code?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #878: Get
the MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r389975868
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,89 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ // Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ // 1. The fields shall have exactly the same meaning.
+ // 2. The value shall be fully compatible, no additional calculation involved.
+ // 3. Resource Config items have a high priority.
+
+ // Return a newly constructed resource config to avoid the original value be modified.
+ ResourceConfig mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name(),
+ idealState.getInstanceGroupTag());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name(),
+ idealState.getMaxPartitionsPerInstance());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.NUM_PARTITIONS.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfigProperty.NUM_PARTITIONS.name(), idealState.getNumPartitions());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name(),
+ idealState.getStateModelDefRef());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name(),
+ idealState.getStateModelFactoryName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name(),
+ idealState.getReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name())) {
+ mergedZNRecord.setIntField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name(),
+ idealState.getMinActiveReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name())) {
+ mergedZNRecord.setBooleanField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name(),
+ idealState.isEnabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name(),
+ idealState.getResourceGroupName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name(),
+ idealState.getResourceType());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name(),
+ idealState.isExternalViewDisabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name(),
+ idealState.isDelayRebalanceEnabled());
+ }
Review comment:
I tried.
1. Although the field keys are the same, the get logic is different. If I copypaste directly based on the field names, I might change the IS logic. For example, getReplicas(). It would be safer that I call the explicit IdealState get methods.
Unfortunately, this will lead to a similar code. But if we take this as a temporary backward compatible workaround, I think it is acceptable. For the correctness.
2. The 2 lists are not exactly the same. DELAY_REBALANCE_DISABLED in IS and DELAY_REBALANCE_ENABLED in the ResourceConfig.
To reduce this complex, I propose to add some setXXXIfAbsent in the ZNRecord class. Will update soon.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #878: Get
the MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r390631345
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,72 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ *
+ * Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ * 1. The fields shall have exactly the same meaning.
+ * 2. The value shall be fully compatible, no additional calculation involved.
+ * 3. Resource Config items have a high priority.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ if (idealState == null) {
+ return resourceConfig;
+ }
+ ResourceConfig mergedResourceConfig;
+ if (resourceConfig != null) {
+ if (!resourceConfig.getResourceName().equals(idealState.getResourceName())) {
+ throw new IllegalArgumentException(String.format(
+ "Cannot merge the IdealState of resource %s with the ResourceConfig of resource %s",
+ resourceConfig.getResourceName(), idealState.getResourceName()));
+ }
+ // Copy the resource config to avoid the original value being modified unexpectedly.
+ mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ } else {
+ // If no resource config specified, construct one based on the Idealstate.
+ mergedResourceConfig = new ResourceConfig(idealState.getResourceName());
+ }
+ // Fill the compatible Idealstate fields to the ResourceConfig if possible.
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
Review comment:
Please check my reply to Hunter's question. The get methods are different.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #878: Get the
MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#issuecomment-597779672
This PR is ready to be merged, approved by @narendly
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #878: Get
the MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r389972403
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,89 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ // Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ // 1. The fields shall have exactly the same meaning.
+ // 2. The value shall be fully compatible, no additional calculation involved.
+ // 3. Resource Config items have a high priority.
+
+ // Return a newly constructed resource config to avoid the original value be modified.
+ ResourceConfig mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name(),
+ idealState.getInstanceGroupTag());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name(),
+ idealState.getMaxPartitionsPerInstance());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.NUM_PARTITIONS.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfigProperty.NUM_PARTITIONS.name(), idealState.getNumPartitions());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name(),
+ idealState.getStateModelDefRef());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name(),
+ idealState.getStateModelFactoryName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name(),
+ idealState.getReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name())) {
+ mergedZNRecord.setIntField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name(),
+ idealState.getMinActiveReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name())) {
+ mergedZNRecord.setBooleanField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name(),
+ idealState.isEnabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name(),
+ idealState.getResourceGroupName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name(),
+ idealState.getResourceType());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name(),
+ idealState.isExternalViewDisabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name(),
+ idealState.isDelayRebalanceEnabled());
+ }
Review comment:
Some names are different in two list, . And the types are different.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #878: Get
the MinActiveReplica from Resource Config and Idea State
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #878: Get the MinActiveReplica from Resource Config and Idea State
URL: https://github.com/apache/helix/pull/878#discussion_r389972403
##########
File path: helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java
##########
@@ -842,5 +841,89 @@ public ResourceConfig build() {
_mapFields, _p2pMessageEnabled, _partitionCapacityMap);
}
}
+
+ /**
+ * For backward compatibility, propagate the critical simple fields from the IdealState to
+ * the Resource Config.
+ * Eventually, Resource Config should be the only metadata node that contains the required information.
+ */
+ public static ResourceConfig mergeIdealStateWithResourceConfig(
+ final ResourceConfig resourceConfig, final IdealState idealState) {
+ // Note that the config fields get updated in this method shall be fully compatible with ones in the IdealState.
+ // 1. The fields shall have exactly the same meaning.
+ // 2. The value shall be fully compatible, no additional calculation involved.
+ // 3. Resource Config items have a high priority.
+
+ // Return a newly constructed resource config to avoid the original value be modified.
+ ResourceConfig mergedResourceConfig = new ResourceConfig(resourceConfig.getRecord());
+ ZNRecord mergedZNRecord = mergedResourceConfig.getRecord();
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.INSTANCE_GROUP_TAG.name(),
+ idealState.getInstanceGroupTag());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfig.ResourceConfigProperty.MAX_PARTITIONS_PER_INSTANCE.name(),
+ idealState.getMaxPartitionsPerInstance());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.NUM_PARTITIONS.name())) {
+ mergedZNRecord
+ .setIntField(ResourceConfigProperty.NUM_PARTITIONS.name(), idealState.getNumPartitions());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_DEF_REF.name(),
+ idealState.getStateModelDefRef());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAME.name(),
+ idealState.getStateModelFactoryName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.REPLICAS.name(),
+ idealState.getReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name())) {
+ mergedZNRecord.setIntField(ResourceConfig.ResourceConfigProperty.MIN_ACTIVE_REPLICAS.name(),
+ idealState.getMinActiveReplicas());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name())) {
+ mergedZNRecord.setBooleanField(ResourceConfig.ResourceConfigProperty.HELIX_ENABLED.name(),
+ idealState.isEnabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name())) {
+ mergedZNRecord
+ .setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_GROUP_NAME.name(),
+ idealState.getResourceGroupName());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name())) {
+ mergedZNRecord.setSimpleField(ResourceConfig.ResourceConfigProperty.RESOURCE_TYPE.name(),
+ idealState.getResourceType());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.EXTERNAL_VIEW_DISABLED.name(),
+ idealState.isExternalViewDisabled());
+ }
+ if (null == mergedZNRecord
+ .getSimpleField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name())) {
+ mergedZNRecord
+ .setBooleanField(ResourceConfig.ResourceConfigProperty.DELAY_REBALANCE_ENABLED.name(),
+ idealState.isDelayRebalanceEnabled());
+ }
Review comment:
Some names are different in two list, . And the types are different.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org