[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/helix/pull/65


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[pnarayanan]

Github user pnarayanan commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95283527
  
--- Diff: helix-core/src/main/java/org/apache/helix/HelixAdmin.java ---
@@ -51,6 +51,16 @@
   InstanceConfig getInstanceConfig(String clusterName, String 
instanceName);
 
   /**
+   * Set the instance config of an existing instance under the given 
cluster.
+   * @param clusterName the name of the cluster to which this instance 
belongs.
+   * @param instanceName the name of this instance.
+   * @param instanceConfig the new {@link InstanceConfig} that will 
replace the current one
+   *   associated with this instance.
+   * @return true if the operation was successful; false otherwise.
+   */
+  boolean setInstanceConfig(String clusterName, String instanceName, 
InstanceConfig instanceConfig);
--- End diff --

Could do that, but doesn't `setInstanceConfig(clusterName, instanceConfig)` 
feel a bit odd as an API?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[pnarayanan]

Github user pnarayanan commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95282358
  
--- Diff: 
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String 
clusterName, String instanceName)
   }
 
   @Override
+  public boolean setInstanceConfig(String clusterName, String 
instanceName,InstanceConfig instanceConfig) {
--- End diff --

Was looking for the style sheet - could you point me to it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[dasahcc]

Github user dasahcc commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95282190
  
--- Diff: 
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String 
clusterName, String instanceName)
   }
 
   @Override
+  public boolean setInstanceConfig(String clusterName, String 
instanceName,InstanceConfig instanceConfig) {
--- End diff --

Same here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[dasahcc]

Github user dasahcc commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95282053
  
--- Diff: helix-core/src/main/java/org/apache/helix/HelixAdmin.java ---
@@ -51,6 +51,16 @@
   InstanceConfig getInstanceConfig(String clusterName, String 
instanceName);
 
   /**
+   * Set the instance config of an existing instance under the given 
cluster.
+   * @param clusterName the name of the cluster to which this instance 
belongs.
+   * @param instanceName the name of this instance.
+   * @param instanceConfig the new {@link InstanceConfig} that will 
replace the current one
+   *   associated with this instance.
+   * @return true if the operation was successful; false otherwise.
+   */
+  boolean setInstanceConfig(String clusterName, String instanceName, 
InstanceConfig instanceConfig);
--- End diff --

The instanceName argument is not necessary since it can be get from 
instanceConfig.getInstanceName()


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[kishoreg]

Github user kishoreg commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95282034
  
--- Diff: 
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String 
clusterName, String instanceName)
   }
 
   @Override
+  public boolean setInstanceConfig(String clusterName, String 
instanceName,InstanceConfig instanceConfig) {
+String instanceConfigPath =
+PropertyPathConfig.getPath(PropertyType.CONFIGS, clusterName, 
ConfigScopeProperty.PARTICIPANT.toString(),
+instanceName);
+if (!_zkClient.exists(instanceConfigPath)) {
+  throw new HelixException("instance" + instanceName + " does not 
exist in cluster " + clusterName);
+}
+
+HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName, new 
ZkBaseDataAccessor(_zkClient));
+Builder keyBuilder = accessor.keyBuilder();
+return accessor.setProperty(keyBuilder.instanceConfig(instanceName), 
instanceConfig);
--- End diff --

this will override the existing configuration. Helix relies on HELIX_HOST 
and HELIX_PORT in this instanceConfig. We have two options here
-- Ensure that these two properties are set. If not, copy them from 
previous instanceConfig.
-- Change the method name to updateInstanceConfig and merge the old and new 
instanceConfigs. We can do this using 
oldInstanceConfig.getRecord().merge(newInstanceConfig.getRecord()); 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[kishoreg]

Github user kishoreg commented on a diff in the pull request:

https://github.com/apache/helix/pull/65#discussion_r95281496
  
--- Diff: 
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String 
clusterName, String instanceName)
   }
 
   @Override
+  public boolean setInstanceConfig(String clusterName, String 
instanceName,InstanceConfig instanceConfig) {
--- End diff --

Please format using the style sheet.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...

[pnarayanan]

GitHub user pnarayanan opened a pull request:

https://github.com/apache/helix/pull/65

[HELIX-651] Add a method in HelixAdmin to set the InstanceConfig

- Add a setInstanceConfig() method in HelixAdmin interface
- Add an implementation for the same in ZkHelixAdmin
- Add a test in TestZkHelixAdmin

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/pnarayanan/helix setInstanceConfig

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/helix/pull/65.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #65


commit b7872d463a1dc7cf9d18369df9008f44cdd27a3a
Author: Priyesh Narayanan 
Date:   2017-01-10T00:45:46Z

[HELIX-651] Add a method in HelixAdmin to set the InstanceConfig of an 
existing instance

- Add a setInstanceConfig() method in HelixAdmin interface
- Add an implementation for the same in ZkHelixAdmin
- Add a test in TestZkHelixAdmin




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---