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/04 23:03:23 UTC

[GitHub] [helix] mgao0 opened a new pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

mgao0 opened a new pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859
 
 
   ### Description
   
   - [ ] Here are some details about my PR, including screenshots of any UI changes:
   
   Currently the update customized state method is synchronized, and this PR make the delete method synchronized as well to prevent the thread interference.
   
   ### Tests
   
   - [ ] The following tests are written for this issue:
   
   (List the names of added unit/integration tests)
   
   - [ ] The following is the result of the "mvn test" command on the appropriate module:
   ===============================================
   Default Suite
   Total tests run: 6, Failures: 0, Skips: 0
   ===============================================
   
   ### 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"
   
   ### 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] mgao0 commented on issue #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859#issuecomment-594924746
 
 
   This PR is ready to be merged, approved by @zhangmeng916 .

----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859#discussion_r388460083
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -113,7 +113,7 @@ public CustomizedState getCustomizedState(String customizedStateName, String res
   /**
    * Delete the customized state for a specified resource and a specified partition
    */
-  public void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
+  public synchronized void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
 
 Review comment:
   There can be multiple threads that try to delete partitions in a same resource. Do we want to leave the currency to the Zookeeper or maybe we can prevent it from the beginning. There are some similar pattern in the code base. We can discuss to decide whether we want to follow.

----------------------------------------------------------------
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] mgao0 commented on issue #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-597773169
 
 
   This PR is ready to be merged, approved by @jiajunwang.
   Final commit message: 
   Title: Use updater to update customized state for concurrency control
   Body: Currently the update customized state method is made synchronized for concurrency control. This commit modifies the implementation of update to leave the responsibility of concurrency control to ZooKeeper by using updater to update the customize state. With delete method already implemented with updater, we can prevent unexpected change of the customize state data.

----------------------------------------------------------------
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] mgao0 edited a comment on issue #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
mgao0 edited a comment on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-596850703
 
 
   This PR is ready to be merged, approved by @jiajunwang.
   Final commit message: Use updater to update customized 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:
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] mgao0 commented on issue #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-596850703
 
 
   This PR is ready to be merged, approved by @jiajunwang.

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r389920071
 
 

 ##########
 File path: helix-core/src/test/java/org/apache/helix/integration/paticipant/TestCustomizedStateUpdate.java
 ##########
 @@ -113,40 +143,176 @@ public void testUpdateCustomizedState() throws Exception {
     Map<String, String> stateMap2 = new HashMap<>();
     stateMap2.put("PREVIOUS_STATE", "STARTED");
     stateMap2.put("CURRENT_STATE", "END_OF_PUSH_RECEIVED");
-    mockProvider.updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2,
-        stateMap2);
+    _mockProvider
+        .updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2, stateMap2);
 
-    customizedState = mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
+    customizedState = _mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
     Assert.assertNotNull(customizedState);
     Assert.assertEquals(customizedState.getId(), RESOURCE_NAME);
     mapView = customizedState.getRecord().getMapFields();
     Assert.assertEquals(mapView.keySet().size(), 2);
-    Assert.assertEqualsNoOrder(mapView.keySet().toArray(), new String[] {
-        PARTITION_NAME1, PARTITION_NAME2
-    });
+    Assert.assertEqualsNoOrder(mapView.keySet().toArray(),
+        new String[]{PARTITION_NAME1, PARTITION_NAME2});
 
-    Map<String, String> partitionMap1 = mockProvider
+    Map<String, String> partitionMap1 = _mockProvider
         .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1);
     Assert.assertEquals(partitionMap1.keySet().size(), 2);
     Assert.assertEquals(partitionMap1.get("PREVIOUS_STATE"), "END_OF_PUSH_RECEIVED");
     Assert.assertEquals(partitionMap1.get("CURRENT_STATE"), "COMPLETED");
 
-    Map<String, String> partitionMap2 = mockProvider
+    Map<String, String> partitionMap2 = _mockProvider
         .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2);
     Assert.assertEquals(partitionMap2.keySet().size(), 2);
     Assert.assertEquals(partitionMap2.get("PREVIOUS_STATE"), "STARTED");
     Assert.assertEquals(partitionMap2.get("CURRENT_STATE"), "END_OF_PUSH_RECEIVED");
 
     // test delete customized state for a partition
-    mockProvider.deletePerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME,
-        PARTITION_NAME1);
-    customizedState = mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
+    _mockProvider
+        .deletePerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1);
+    customizedState = _mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
     Assert.assertNotNull(customizedState);
     Assert.assertEquals(customizedState.getId(), RESOURCE_NAME);
     mapView = customizedState.getRecord().getMapFields();
     Assert.assertEquals(mapView.keySet().size(), 1);
     Assert.assertEquals(mapView.keySet().iterator().next(), PARTITION_NAME2);
+  }
+
+  @Test
+  public void testUpdateSinglePartitionCustomizedState() {
+    _mockProvider.updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1,
+        PARTITION_STATE);
+
+    // get customized state
+    CustomizedState customizedState =
+        _mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
+    Assert.assertEquals(
+        customizedState.getPartitionStateMap(CustomizedState.CustomizedStateProperty.CURRENT_STATE)
+            .size(), 1);
+    Map<String, String> map = new HashMap<>();
+    map.put(PARTITION_NAME1, null);
+    Assert.assertEquals(customizedState
+        .getPartitionStateMap(CustomizedState.CustomizedStateProperty.PREVIOUS_STATE), map);
+    Assert.assertEquals(
+        customizedState.getPartitionStateMap(CustomizedState.CustomizedStateProperty.START_TIME),
+        map);
+    Assert.assertEquals(
+        customizedState.getPartitionStateMap(CustomizedState.CustomizedStateProperty.END_TIME),
+        map);
+    Assert.assertEquals(customizedState.getState(PARTITION_NAME1), PARTITION_STATE);
+    Assert.assertNull(customizedState.getState(PARTITION_NAME2));
+    Assert.assertTrue(customizedState.isValid());
+
+    // get per partition customized state
+    map = new HashMap<>();
+    map.put(CustomizedState.CustomizedStateProperty.CURRENT_STATE.name(), PARTITION_STATE);
+    Map<String, String> partitionCustomizedState = _mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1);
+    Assert.assertEquals(partitionCustomizedState, map);
+    Assert.assertNull(_mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2));
+  }
+
+  @Test
+  public void testUpdateSinglePartitionCustomizedStateWithNullField() {
+    _mockProvider
+        .updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1, (String) null);
+
+    // get customized state
+    CustomizedState customizedState =
+        _mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
+    Map<String, String> map = new HashMap<>();
+    map.put(PARTITION_NAME1, null);
+    Assert.assertEquals(
+        customizedState.getPartitionStateMap(CustomizedState.CustomizedStateProperty.CURRENT_STATE),
+        map);
+    Assert.assertEquals(customizedState.getState(PARTITION_NAME1), null);
+    Assert.assertTrue(customizedState.isValid());
+
+    // get per partition customized state
+    map = new HashMap<>();
+    map.put(CustomizedState.CustomizedStateProperty.CURRENT_STATE.name(), null);
+    Map<String, String> partitionCustomizedState = _mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1);
+    Assert.assertEquals(partitionCustomizedState, map);
+    Assert.assertNull(_mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2));
+  }
+
+  @Test
+  public void testUpdateCustomizedStateWithEmptyMap() {
+    _mockProvider.updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1,
+        new HashMap<>());
+
+    // get customized state
+    CustomizedState customizedState =
+        _mockProvider.getCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME);
+    Assert.assertNull(customizedState.getState(PARTITION_NAME1));
+    Map<String, String> partitionStateMap =
+        customizedState.getPartitionStateMap(CustomizedState.CustomizedStateProperty.CURRENT_STATE);
+    Assert.assertNotNull(partitionStateMap);
+    Assert.assertTrue(partitionStateMap.containsKey(PARTITION_NAME1));
+    Assert.assertNull(partitionStateMap.get(PARTITION_NAME1));
+    Assert.assertNull(customizedState.getState(PARTITION_NAME1));
+    Assert.assertFalse(partitionStateMap.containsKey(PARTITION_NAME2));
+    Assert.assertTrue(customizedState.isValid());
+
+    // get per partition customized state
+    Map<String, String> partitionCustomizedState = _mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1);
+    Assert.assertEquals(partitionCustomizedState.size(), 0);
+    Assert.assertNull(_mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2));
+  }
+
+  @Test
+  public void testDeleteNonExistingPerPartitionCustomizedState() {
+    _mockProvider.updateCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1,
+        PARTITION_STATE);
+    _mockProvider
+        .deletePerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2);
+    Assert.assertNotNull(_mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME1));
+    Assert.assertNull(_mockProvider
+        .getPerPartitionCustomizedState(CUSTOMIZE_STATE_NAME, RESOURCE_NAME, PARTITION_NAME2));
+  }
+
+  @Test
+  public void testSimultaneousUpdateCustomizedState() {
+    int n = 10;
 
 Review comment:
   Not a good idea to use 'n's like this - please either make it a int constant or make it more descriptive.

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-597454079
 
 
   > This PR is ready to be merged, approved by @jiajunwang.
   > Final commit message: Use updater to update customized state
   
   @mgao0 Could you please at least include the purpose of this change in the message? Thanks. More information would be better.

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r390023296
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -61,29 +61,16 @@ public synchronized void updateCustomizedState(String customizedStateName, Strin
    * Update a specific customized state based on the resource name and partition name. The
    * customized state is input as a map
    */
-  public synchronized void updateCustomizedState(String customizedStateName, String resourceName,
+  public void updateCustomizedState(String customizedStateName, String resourceName,
       String partitionName, Map<String, String> customizedStateMap) {
     PropertyKey.Builder keyBuilder = _helixDataAccessor.keyBuilder();
     PropertyKey propertyKey =
         keyBuilder.customizedState(_instanceName, customizedStateName, resourceName);
     ZNRecord record = new ZNRecord(resourceName);
-    Map<String, Map<String, String>> mapFields = new HashMap<>();
-    CustomizedState existingState = getCustomizedState(customizedStateName, resourceName);
-    if (existingState != null
-        && existingState.getRecord().getMapFields().containsKey(partitionName)) {
-      Map<String, String> existingMap = new HashMap<>();
 
 Review comment:
   It is not clear what the original code tries to do here. It seems trying to merge the map. But not really doing so. Can you confirm the original logic is right or not?
   
   The concern is that the default update method is doing deep merge. That is if you have an existing state map with some stale states, even after merging, those stale states will be kept in the map. But I assume we should clean those stale states for updating?

----------------------------------------------------------------
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] mgao0 commented on issue #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-595976098
 
 
   This PR is ready to be merged, approved by @zhangmeng916 

----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r390031835
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -61,29 +61,16 @@ public synchronized void updateCustomizedState(String customizedStateName, Strin
    * Update a specific customized state based on the resource name and partition name. The
    * customized state is input as a map
    */
-  public synchronized void updateCustomizedState(String customizedStateName, String resourceName,
+  public void updateCustomizedState(String customizedStateName, String resourceName,
       String partitionName, Map<String, String> customizedStateMap) {
     PropertyKey.Builder keyBuilder = _helixDataAccessor.keyBuilder();
     PropertyKey propertyKey =
         keyBuilder.customizedState(_instanceName, customizedStateName, resourceName);
     ZNRecord record = new ZNRecord(resourceName);
-    Map<String, Map<String, String>> mapFields = new HashMap<>();
-    CustomizedState existingState = getCustomizedState(customizedStateName, resourceName);
-    if (existingState != null
-        && existingState.getRecord().getMapFields().containsKey(partitionName)) {
-      Map<String, String> existingMap = new HashMap<>();
 
 Review comment:
   Yeah, we do want the full map like instead of replacing the previous map. Because for customized state, the key are fixed, e.g. "previous state", "current state", etc. We do not allow user to delete entries, and if they do not have value, the entry will have null value. 
   
   For this API, it's updating the per partition customized state, not adding new type of customized states.
   So it's a definite map of
   {"start time": xxx
   "end time:xxx
   "previous state":xxx
   "current state":xxx
   }
   and customers update one entry or the full map. 

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r390034663
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -61,29 +61,16 @@ public synchronized void updateCustomizedState(String customizedStateName, Strin
    * Update a specific customized state based on the resource name and partition name. The
    * customized state is input as a map
    */
-  public synchronized void updateCustomizedState(String customizedStateName, String resourceName,
+  public void updateCustomizedState(String customizedStateName, String resourceName,
       String partitionName, Map<String, String> customizedStateMap) {
     PropertyKey.Builder keyBuilder = _helixDataAccessor.keyBuilder();
     PropertyKey propertyKey =
         keyBuilder.customizedState(_instanceName, customizedStateName, resourceName);
     ZNRecord record = new ZNRecord(resourceName);
-    Map<String, Map<String, String>> mapFields = new HashMap<>();
-    CustomizedState existingState = getCustomizedState(customizedStateName, resourceName);
-    if (existingState != null
-        && existingState.getRecord().getMapFields().containsKey(partitionName)) {
-      Map<String, String> existingMap = new HashMap<>();
 
 Review comment:
   I see. That makes sense.

----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r390025024
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -61,29 +61,16 @@ public synchronized void updateCustomizedState(String customizedStateName, Strin
    * Update a specific customized state based on the resource name and partition name. The
    * customized state is input as a map
    */
-  public synchronized void updateCustomizedState(String customizedStateName, String resourceName,
+  public void updateCustomizedState(String customizedStateName, String resourceName,
       String partitionName, Map<String, String> customizedStateMap) {
     PropertyKey.Builder keyBuilder = _helixDataAccessor.keyBuilder();
     PropertyKey propertyKey =
         keyBuilder.customizedState(_instanceName, customizedStateName, resourceName);
     ZNRecord record = new ZNRecord(resourceName);
-    Map<String, Map<String, String>> mapFields = new HashMap<>();
-    CustomizedState existingState = getCustomizedState(customizedStateName, resourceName);
-    if (existingState != null
-        && existingState.getRecord().getMapFields().containsKey(partitionName)) {
-      Map<String, String> existingMap = new HashMap<>();
 
 Review comment:
   So this is user to update a customized state for a certain partition. I don't think we have the knowledge to clean up stale states, meaning if the user only update "CURRENT_STATE" of customized state, we update it, and keep the other fields unchanged. If they would like to do a full update, they will use the other API and provide the full map, and the original map will be overwritten.

----------------------------------------------------------------
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] mgao0 removed a comment on issue #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
mgao0 removed a comment on issue #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#issuecomment-595976098
 
 
   This PR is ready to be merged, approved by @zhangmeng916 

----------------------------------------------------------------
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 #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859#issuecomment-595375938
 
 
   > @jiajunwang Please see line 76, we rely on the record we get from zk for the update, and set the record to the local map to update the znode. So I think if there is any change during get and update, the data would be messed up.
   
   I see your point. But if we are using the "updater", and use it in the correct way, I believe we don't need this additional sync. Actually, let's address this problem in this PR. Make the logic around 76 use updater.

----------------------------------------------------------------
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 #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859#discussion_r388464874
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -113,7 +113,7 @@ public CustomizedState getCustomizedState(String customizedStateName, String res
   /**
    * Delete the customized state for a specified resource and a specified partition
    */
-  public void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
+  public synchronized void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
 
 Review comment:
   ZK is the source of truth. Even we control the concurrency in one single node, we can not prevent multiple nodes concurrent access. So leverage ZK capability is the right way.
   In detail, updater prevents unexpected overwrite. So I don't think the additional check is necessary. Unless we have a concrete counter-example.

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r390030055
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -61,29 +61,16 @@ public synchronized void updateCustomizedState(String customizedStateName, Strin
    * Update a specific customized state based on the resource name and partition name. The
    * customized state is input as a map
    */
-  public synchronized void updateCustomizedState(String customizedStateName, String resourceName,
+  public void updateCustomizedState(String customizedStateName, String resourceName,
       String partitionName, Map<String, String> customizedStateMap) {
     PropertyKey.Builder keyBuilder = _helixDataAccessor.keyBuilder();
     PropertyKey propertyKey =
         keyBuilder.customizedState(_instanceName, customizedStateName, resourceName);
     ZNRecord record = new ZNRecord(resourceName);
-    Map<String, Map<String, String>> mapFields = new HashMap<>();
-    CustomizedState existingState = getCustomizedState(customizedStateName, resourceName);
-    if (existingState != null
-        && existingState.getRecord().getMapFields().containsKey(partitionName)) {
-      Map<String, String> existingMap = new HashMap<>();
 
 Review comment:
   I don't think you get my point, or maybe I did not get the point. Anyway, let's go with an example,
   1. The original map {"CSTypeA":"foo", "CSTypeB":"bar"}
   2. User try to update with map {"CSTypeC":"foo", "CSTypeD":"bar"}
   3. I assume the expect result for the custmize state is {"CSTypeC":"foo", "CSTypeD":"bar"}.
   4. But really by the default merge method we get: {"CSTypeA":"foo", "CSTypeB":"bar", "CSTypeC":"foo", "CSTypeD":"bar"}
   CSTypeA and CSTypeB is the stale state that I meant.

----------------------------------------------------------------
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859
 
 
   

----------------------------------------------------------------
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 #859: Add 'synchronized' key word to delete method in CustomizedStateProvider

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #859: Add 'synchronized' key word to delete method in CustomizedStateProvider
URL: https://github.com/apache/helix/pull/859#discussion_r388116466
 
 

 ##########
 File path: helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProvider.java
 ##########
 @@ -113,7 +113,7 @@ public CustomizedState getCustomizedState(String customizedStateName, String res
   /**
    * Delete the customized state for a specified resource and a specified partition
    */
-  public void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
+  public synchronized void deletePerPartitionCustomizedState(String customizedStateName, String resourceName,
 
 Review comment:
   Why we need the synchronized keyword at the first place? ZK should be the one who takes care of concurrent control, 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:
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 #859: Update customize state using updater

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #859: Update customize state using updater
URL: https://github.com/apache/helix/pull/859#discussion_r389919470
 
 

 ##########
 File path: helix-core/src/test/java/org/apache/helix/integration/paticipant/TestCustomizedStateUpdate.java
 ##########
 @@ -19,54 +19,84 @@
  * under the License.
  */
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.stream.Collectors;
+
 import org.apache.helix.HelixDataAccessor;
 import org.apache.helix.HelixManager;
 import org.apache.helix.HelixManagerFactory;
 import org.apache.helix.InstanceType;
 import org.apache.helix.PropertyKey;
+import org.apache.helix.TestHelper;
 import org.apache.helix.customizedstate.CustomizedStateProvider;
 import org.apache.helix.customizedstate.CustomizedStateProviderFactory;
 import org.apache.helix.integration.common.ZkStandAloneCMTestBase;
 import org.apache.helix.model.CustomizedState;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+
 public class TestCustomizedStateUpdate extends ZkStandAloneCMTestBase {
   private static Logger LOG = LoggerFactory.getLogger(TestCustomizedStateUpdate.class);
   private final String CUSTOMIZE_STATE_NAME = "testState1";
   private final String PARTITION_NAME1 = "testPartition1";
   private final String PARTITION_NAME2 = "testPartition2";
   private final String RESOURCE_NAME = "testResource1";
+  private final String PARTITION_STATE = "partitionState";
+  private static HelixManager _manager;
+  private static CustomizedStateProvider _mockProvider;
 
-  @Test
-  public void testUpdateCustomizedState() throws Exception {
-    HelixManager manager = HelixManagerFactory.getZKHelixManager(CLUSTER_NAME, "admin",
-        InstanceType.ADMINISTRATOR, ZK_ADDR);
-    manager.connect();
+  @BeforeClass
+  public void beforeClass() throws Exception {
+    super.beforeClass();
+    _manager = HelixManagerFactory
+        .getZKHelixManager(CLUSTER_NAME, "admin", InstanceType.ADMINISTRATOR, ZK_ADDR);
+    _manager.connect();
     _participants[0].connect();
+    _mockProvider = CustomizedStateProviderFactory.getInstance()
+        .buildCustomizedStateProvider(_manager, _participants[0].getInstanceName());
+  }
+
+  @AfterClass
+  public void afterClass() throws Exception {
+    super.afterClass();
+    _manager.disconnect();
+  }
 
-    HelixDataAccessor dataAccessor = manager.getHelixDataAccessor();
+  @BeforeMethod
+  public void beforeMethod() {
+    HelixDataAccessor dataAccessor = _manager.getHelixDataAccessor();
 
 Review comment:
   This could go to beforeClass, 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:
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