You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/06/27 03:28:55 UTC

[GitHub] [incubator-pinot] yupeng9 opened a new pull request #5631: Add the message constraint on all instances in Helix

yupeng9 opened a new pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631


   ## Description
   Add a constraint to the message queue for all instances in Helix, with a large default value of 100000.
   
   This throttles the total number of messages to avoid the sudden surge of zk nodes, which could be larger than the zookeeper buffer size. Today, when the buffer overflows, the controller and server run into exception and cannot proceed with any state change, until the zk nodes are trimmed manually.
   
   ![image](https://user-images.githubusercontent.com/13425258/85913483-2a695d80-b7ea-11ea-8175-8ff12a6e0f33.png)
   The diagram shows how this constraint works. Without constraint, the first spike of ~3k messages within 1 minute is from the state transition messages generated under this instance after restarting a server. With the constraint, the zk node increase is capped at 1k for that instance. 
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   * [ ] No
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   * [ ] No
   
   Does this PR otherwise need attention when creating release notes? Things to consider:
   * [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
   ## Release Notes
   - New configuration options
   A new configuration `pinot.helix.instance.messages.max` is added for the constraint, with default value of 100k
   
   ## Documentation
   If you have introduced a new feature or configuration, please add it to the documentation as well.
   See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-658431790


   my vote 100k, not many will reach that scale but in case they do, this should prevent an outage in case a pinot server misbehaves.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631


   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657182505


   @mcvsubbu did anyone from Helix team look into this? 


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r454020153



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,20 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_STATE_TRANSITIONS);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = new HashMap<>();
+    constraintAttributes.put(ClusterConstraints.ConstraintAttribute.INSTANCE, ".*");
+    constraintAttributes
+        .put(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE, Message.MessageType.STATE_TRANSITION.name());
+    ConstraintItem constraintItem = new ConstraintItem(constraintAttributes, maxMessageLimit);
+
+    _helixControllerManager.getClusterManagmentTool()

Review comment:
       @mcvsubbu with Junkai's assessment, do you still have concerns on the current approach, given it's a safe change for better zookeeper scalability? 
   I prefer the current way since it simplifies the configuration story, i.e. no need to use external Helix admin to clean up constraint etc.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] dasahcc commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
dasahcc commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657737205


   For this PR, from Helix dev perspective, it will not impact the correctness but could apply the throttling logic to make the bootstrap longer. But also it brings more stability of the Zookeeper. As long as Pinot is fine with that, this is a good change. 
   
   @yupeng9 Helix provide helix-rest module for supporting lot of REST APIs and running as a separated service. If you would like to use some advanced API, you can try that part.
    


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657661567


   we have too many configs which enable/disable other configs. These things never get cleaned up. Also, this setting is applied only to the newly created cluster and should not matter to the existing cluster.
   
   The right thing to do here is to provide an admin API to add/remove/edit the constraint in Helix.
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-652626786


   > Great work digging this out from the Helix code (I'm not able to find this from Helix documentation).
   > 
   > LGTM in general. I suggest renaming all `maxMessages` to `maxStateTransitions` because we are not actually limiting the messages.
   
   Thanks for the code review. Addressed some of the comments


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r454050108



##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeTest.java
##########
@@ -248,6 +253,16 @@ public void testPinotOnlyController() {
     helixOnlyController.stop();
   }
 
+  private void checkHelixConstraints(HelixAdmin helixAdmin) {
+    ClusterConstraints constraints =
+        helixAdmin.getConstraints(getHelixClusterName(), ClusterConstraints.ConstraintType.MESSAGE_CONSTRAINT);
+    ConstraintItem item = constraints.getConstraintItem("MaxStateTransitionsPerInstance");
+    Assert.assertEquals(".*", item.getAttributeValue(ClusterConstraints.ConstraintAttribute.INSTANCE));

Review comment:
       Please reverse the arguments for all the asserts, where the first should be actual, and the second should be expected.

##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeTest.java
##########
@@ -248,6 +253,16 @@ public void testPinotOnlyController() {
     helixOnlyController.stop();
   }
 
+  private void checkHelixConstraints(HelixAdmin helixAdmin) {
+    ClusterConstraints constraints =
+        helixAdmin.getConstraints(getHelixClusterName(), ClusterConstraints.ConstraintType.MESSAGE_CONSTRAINT);
+    ConstraintItem item = constraints.getConstraintItem("MaxStateTransitionsPerInstance");
+    Assert.assertEquals(".*", item.getAttributeValue(ClusterConstraints.ConstraintAttribute.INSTANCE));
+    Assert
+        .assertEquals("STATE_TRANSITION", item.getAttributeValue(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE));

Review comment:
       ```suggestion
           .assertEquals(item.getAttributeValue(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE), Message.MessageType.STATE_TRANSITION.name());
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -109,6 +109,8 @@
     public static final String CONFIG_OF_BROKER_FLAPPING_TIME_WINDOW_MS = "pinot.broker.flapping.timeWindowMs";
     public static final String CONFIG_OF_SERVER_FLAPPING_TIME_WINDOW_MS = "pinot.server.flapping.timeWindowMs";
     public static final String CONFIG_OF_MINION_FLAPPING_TIME_WINDOW_MS = "pinot.minion.flapping.timeWindowMs";
+    public static final String CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS = "pinot.helix.instance.state.transitions.max";

Review comment:
       ```suggestion
       public static final String CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS = "pinot.helix.instance.maxStateTransitions";
   ```

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,20 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS,

Review comment:
       ```suggestion
       String maxStateTransitions = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS,
   ```




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r449711597



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,20 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_STATE_TRANSITIONS);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = new HashMap<>();
+    constraintAttributes.put(ClusterConstraints.ConstraintAttribute.INSTANCE, ".*");
+    constraintAttributes
+        .put(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE, Message.MessageType.STATE_TRANSITION.name());
+    ConstraintItem constraintItem = new ConstraintItem(constraintAttributes, maxMessageLimit);
+
+    _helixControllerManager.getClusterManagmentTool()

Review comment:
       While this may be a good thing to do, we have often experienced issues with helix, especially the undocumented features. Can we at least make this so that if the config is not set, then we leave the behavior like before?
   
   Meanwhile. I have asked someone from the helix team to take a look at this.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657908186


   No concerns, go for it.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r448067581



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -97,6 +103,7 @@
   private static final Long DATA_DIRECTORY_MISSING_VALUE = 1000000L;
   private static final Long DATA_DIRECTORY_EXCEPTION_VALUE = 1100000L;
   private static final String METADATA_EVENT_NOTIFIER_PREFIX = "metadata.event.notifier";
+  private static final String MAX_MESSAGES_PER_INSTANCE_NAME =  "MaxMessagesPerInstance";

Review comment:
       ```suggestion
     private static final String MAX_STATE_TRANSITIONS_PER_INSTANCE =  "MaxStateTransitionsPerInstance";
   ```

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,18 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_MESSAGES,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_MESSAGES);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = Maps.newHashMap();

Review comment:
       No need to use guava for this. We have experienced several backward-incompatible issues with guava.
   ```suggestion
       Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = new HashMap()<>;
   ```

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,18 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_MESSAGES,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_MESSAGES);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = Maps.newHashMap();
+    constraintAttributes.put(ClusterConstraints.ConstraintAttribute.INSTANCE, ".*");
+    constraintAttributes.put(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE, "STATE_TRANSITION");

Review comment:
       ```suggestion
       constraintAttributes.put(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE, Message.MessageType.STATE_TRANSITION.name());
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -109,6 +109,8 @@
     public static final String CONFIG_OF_BROKER_FLAPPING_TIME_WINDOW_MS = "pinot.broker.flapping.timeWindowMs";
     public static final String CONFIG_OF_SERVER_FLAPPING_TIME_WINDOW_MS = "pinot.server.flapping.timeWindowMs";
     public static final String CONFIG_OF_MINION_FLAPPING_TIME_WINDOW_MS = "pinot.minion.flapping.timeWindowMs";
+    public static final String CONFIG_OF_HELIX_INSTANCE_MAX_MESSAGES = "pinot.helix.instance.messages.max";

Review comment:
       Recommend renaming it for clarity and add some comments on how it works (limit the maximum number of state transitions for each instance):
   ```suggestion
       public static final String CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS = "pinot.helix.instance.maxStateTransitions";
   ```

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -350,6 +377,9 @@ private void setUpPinotController() {
       _realtimeSegmentsManager = null;
     }
 
+    // setup constraints
+    setupHelixClusterConstraints();

Review comment:
       We should not set this for Pinot controller but only for Helix controller




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r448581387



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,18 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_MESSAGES,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_MESSAGES);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = Maps.newHashMap();

Review comment:
       Good to know this. thanks




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657981243


   > LGTM with minor comments. Please let me know when you want me to merge it.
   
   Thanks for taking another pass. Addressed the comments


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657751504


   
   > For this PR, from Helix dev perspective, it will not impact the correctness but could apply the throttling logic to make the bootstrap longer. But also it brings more stability of the Zookeeper. As long as Pinot is fine with that, this is a good change.
   
   Thanks for taking a look and the confirmation
   > 
   > BTW, Helix team does suggest to use throttling to controller the number of messages. For the number of the threshold, it is up to Pinot team to discuss.
   > 
   > @yupeng9 Helix provide helix-rest module for supporting lot of REST APIs and running as a separated service. If you would like to use some advanced API, you can try that part.
   
   Yes, I have been using `helix-admin-webapp` around it, and it's handy. Per @kishoreg's suggestion it is a good idea to add `helix-rest` to Pinot's cluster management APIs.
   
   
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-658429465


   @yupeng9 @mcvsubbu Last question before merging: should we put 10M as the default, or 100K as described in the PR?


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-653326881


   This is amazing. Thanks for digging through the code and making this 


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-658423755


   @Jackie-Jiang Got the build pass, feel free to merge


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r449787052



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -202,6 +209,20 @@ private void setupHelixSystemProperties() {
             CommonConstants.Helix.DEFAULT_FLAPPING_TIME_WINDOW_MS));
   }
 
+  private void setupHelixClusterConstraints() {
+    String maxMessageLimit = _config.getString(CommonConstants.Helix.CONFIG_OF_HELIX_INSTANCE_MAX_STATE_TRANSITIONS,
+        CommonConstants.Helix.DEFAULT_HELIX_INSTANCE_MAX_STATE_TRANSITIONS);
+    Map<ClusterConstraints.ConstraintAttribute, String> constraintAttributes = new HashMap<>();
+    constraintAttributes.put(ClusterConstraints.ConstraintAttribute.INSTANCE, ".*");
+    constraintAttributes
+        .put(ClusterConstraints.ConstraintAttribute.MESSAGE_TYPE, Message.MessageType.STATE_TRANSITION.name());
+    ConstraintItem constraintItem = new ConstraintItem(constraintAttributes, maxMessageLimit);
+
+    _helixControllerManager.getClusterManagmentTool()

Review comment:
       Sure. Please have the helix team take a look.
   
   There is some caveat of not adding the constraint when the config is not set: do we need to add logic removing the constraint if the config is not present? Otherwise it's possible that the constraint is left there, when users remove this configuration. What about if there are inconsistent configurations, with the configuration set on one controller but not others (which will lead to race condition)? Usually having the configuration with a default value makes such issues more manageable. 




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] dasahcc edited a comment on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
dasahcc edited a comment on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657737205


   For this PR, from Helix dev perspective, it will not impact the correctness but could apply the throttling logic to make the bootstrap longer. But also it brings more stability of the Zookeeper. As long as Pinot is fine with that, this is a good change. 
   
   BTW, Helix team does suggest to use throttling to controller the number of messages. For the number of the threshold, it is up to Pinot team to discuss.
   
   @yupeng9 Helix provide helix-rest module for supporting lot of REST APIs and running as a separated service. If you would like to use some advanced API, you can try that part.
    


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-653287549


   @mcvsubbu Also increased the default value, let me know if you have additional comments


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-651991905


   Thanks for taking a look.
   
   We also have hosts with more than 100k segments. In fact, this addition of throttling is to protect the servers with large number of segments. The more segments a host has, the more transition messages will be generated during events like restart, and therefore the higher risk the server/controller run out of zookeeper buffer.
   
   The default value of 100k is set corresponding to the default value of `jute.maxbuffer` which is 1MB, but I'm okay with a much higher value like 100MB, if that is a concern.
   
   > This wont work for us since we have more than 100k elements per host. We have not enabled batching yet due to other constraints.
   
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657712885


   I'm flexible on this. As I mentioned in the previous comment, there is a caveat that if the config is not set, then users need to use the Helix admin to remove the constraint with your suggestion. I'm fine with documenting this catch in the documentation. @kishoreg, @Jackie-Jiang are you okay with this?
   
   I understand the worry about the use of the less documented feature in Helix, and I hope the Helix community can address it. I also think a wrapper of Helix admin API in Pinot is a good idea for the ease of cluster management: not need to download and setup the ` helix-admin-webapp`.
   
   But I think it's valuable to keep this as a Pinot config, so users can use it out of the box, given the disastrous outcome this config can prevent.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-658485856


   > The number depends on the capacity of zookeeper to handle transactions. What is to say that 100K is bad and 10K is good? Or that 10M is bad but 1M is not?
   
   One reason to use 100K is that `jute.maxbuffer` defaults to 1MB. So the purpose is to protect the controller/server with the out-of-the-box settings. If we had this config, we could have prevented an outage that brought the entire cluster down for hours.
   > 
   > Why not leave it to whatever helix does by default (i.e. existing behavior) but add a documentation on what helix command sequence to run in order to set the value? (of course, in that case this PR is not needed at all).
   
   I believe an inbuilt configuration gives a better user experience, and reliability assurance.
   
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r449206696



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -350,6 +377,9 @@ private void setUpPinotController() {
       _realtimeSegmentsManager = null;
     }
 
+    // setup constraints
+    setupHelixClusterConstraints();

Review comment:
       `PINOT_ONLY` mode controller does not serve as the Helix controller, so it should not add this constraint to the cluster.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-658450457


   The number depends on the capacity of zookeeper to handle transactions. What is to say that 100K is bad and 10K is good? Or that 10M is bad but 1M is not?
   
   Why not leave it to whatever helix does by default (i.e. existing behavior) but add a documentation on what helix command sequence to run in order to set the value? (of course, in that case this PR is not needed at all).


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r449335792



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -350,6 +377,9 @@ private void setUpPinotController() {
       _realtimeSegmentsManager = null;
     }
 
+    // setup constraints
+    setupHelixClusterConstraints();

Review comment:
       Got it. thanks




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-657655876


   We will give it until EOD for someone in Helix team to review. Meanwhile, please make one change. If the message constraint config is not set, then please do not change helix cluster setting. Let it be whatever default helix has.
   thanks.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-652015621


   We use custom ideal state in pinot, and afaik helix does not support throttling for custom ideal state. I will have someone from helix team comment on the change


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#issuecomment-652050007


   > We use custom ideal state in pinot, and afaik helix does not support throttling for custom ideal state. I will have someone from helix team comment on the change
   
   I think you are referring to `STATE_TRANSITION_THROTTLE_CONFIGS`, which is a cluster config used in `StateTransitionThrottleController `. This uses a cluster constraint, which applies to `MessageThrottleStage` in the pipeline.
   
   I believe @kishoreg can further explain the differences.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5631: Add the message constraint on all instances in Helix

Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #5631:
URL: https://github.com/apache/incubator-pinot/pull/5631#discussion_r448593606



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -350,6 +377,9 @@ private void setUpPinotController() {
       _realtimeSegmentsManager = null;
     }
 
+    // setup constraints
+    setupHelixClusterConstraints();

Review comment:
       But then no constraint is added during `PINOT_ONLY` mode?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org