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/08/25 13:12:23 UTC

[GitHub] [incubator-pinot] adriancole opened a new pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   ## Description
   This starts Broker and Server in parallel when using ServiceManager
   
   Fixes #5876
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? No
   
   Does this PR fix a zero-downtime upgrade introduced earlier? No
   
   Does this PR otherwise need attention when creating release notes? Yes
   
   ## Release Notes
   Broker and Server will now start in parallel when using ServiceManager
   
   ## Documentation
   N/A
   


----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       nm




----------------------------------------------------------------
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] fx19880617 commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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






----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       intentionally left controller out of here as it seemed there was some reason to. Otherwise, I'd like to put it in :D




----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       If user starts broker/server first for a new cluster, then it will fail in-relevant to your changes.




----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   PS this seems it didn't make it to 0.5.0. Can it be added to the next release after that? It will help to not be perpetually on snapshots


----------------------------------------------------------------
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] fx19880617 commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   I think this is good for now.
   I will take a look how to let service manager to connect to helix and decide if it's required to start controller first or it can start everything at once.
   


----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   Thanks @fx19880617 and @kishoreg I will make one last sweep here to make sure if a role crashes the process does. then ask to merge (and ideally cut a snapshot)


----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       I presume you mean inside `startPinotService` as it is a blocking command (then useful for both the parallel and sync paths)




----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   ok here is what it looks like now:
   
   ```
   $ docker logs pinot 2>&1|grep 'INFO.*since launch'
   07:12:45,859 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:254) - Starting a Pinot [CONTROLLER] at 5.448s since launch
   07:12:51,703 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:256) - Started Pinot [CONTROLLER] instance [Controller_f322d4ca725c_9000] at 11.293s since launch
   07:12:51,706 INFO  [Starting BROKER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:254) - Starting a Pinot [BROKER] at 11.296s since launch
   07:12:51,706 INFO  [Starting SERVER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:254) - Starting a Pinot [SERVER] at 11.296s since launch
   07:12:55,523 INFO  [Starting SERVER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:256) - Started Pinot [SERVER] instance [Server_f322d4ca725c_8098] at 15.113s since launch
   07:12:55,593 INFO  [Starting BROKER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:256) - Started Pinot [BROKER] instance [Broker_f322d4ca725c_8099] at 15.183s since launch
   ```


----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       you want to just time each service command here right? this would be different than composite startup time




----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       ok FWIW what I used before was `-verbose:gc` for similar :P you can see time since startup for each GC event, which are often enough to figure out time like this.
   
   ```
   [0.012s][info][gc] Using G1
   [0.066s][info][gc] Periodic GC disabled
   [0.753s][info][gc] GC(0) Pause Young (Normal) (G1 Evacuation Pause) 12M->2M(256M) 7.433ms
   [1.108s][info][gc] GC(1) Pause Young (Normal) (G1 Evacuation Pause) 12M->3M(256M) 9.164ms
   [1.511s][info][gc] GC(2) Pause Young (Concurrent Start) (Metadata GC Threshold) 15M->4M(256M) 6.252ms
   [1.511s][info][gc] GC(3) Concurrent Cycle
   [1.520s][info][gc] GC(3) Pause Remark 4M->4M(256M) 1.834ms
   [1.523s][info][gc] GC(3) Pause Cleanup 4M->4M(256M) 0.172ms
   [1.525s][info][gc] GC(3) Concurrent Cycle 13.819ms
   ```
   
   Ex.
   
   "starting $role at N.NNNs since launch"
   "started/error starting $role at N.NNNs since launch"
   
   




----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       In the context of SM I don't know what to use as instance_id




----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       also I don't know how we check if "cluster is already" so if you feed me a command I can...




----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   Looking at the below, I'm wondering if ServiceManager itself can be deferred to start? It takes 8s
   
   ```
   docker logs pinot 2>&1|grep 'INFO.*since launch'
   11:33:12,151 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [SERVICE_MANAGER] at 0.04s since launch
   11:33:20,299 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [SERVICE_MANAGER] instance [ServiceManager_f0292d096fa4_7098] at 8.189s since launch
   11:33:20,344 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [CONTROLLER] at 8.235s since launch
   11:33:27,010 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [CONTROLLER] instance [Controller_f0292d096fa4_9000] at 14.9s since launch
   11:33:27,012 INFO  [pool-13-thread-1] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [BROKER] at 14.902s since launch
   11:33:27,023 INFO  [pool-13-thread-2] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [SERVER] at 14.914s since launch
   11:33:30,771 INFO  [pool-13-thread-1] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [BROKER] instance [Broker_f0292d096fa4_8099] at 18.696s since launch
   11:33:30,821 INFO  [pool-13-thread-2] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [SERVER] instance [Server_f0292d096fa4_8098] at 18.747s since launch
   ```


----------------------------------------------------------------
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] codecov-commenter edited a comment on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #5917:
URL: https://github.com/apache/incubator-pinot/pull/5917#issuecomment-680784776


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=h1) Report
   > Merging [#5917](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `22.53%`.
   > The diff coverage is `48.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5917/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #5917       +/-   ##
   ===========================================
   - Coverage   66.44%   43.91%   -22.54%     
   ===========================================
     Files        1075     1184      +109     
     Lines       54773    62044     +7271     
     Branches     8168     9479     +1311     
   ===========================================
   - Hits        36396    27248     -9148     
   - Misses      15700    32345    +16645     
   + Partials     2677     2451      -226     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `43.91% <48.81%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `26.66% <0.00%> (-30.48%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [.../org/apache/pinot/common/lineage/LineageEntry.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9MaW5lYWdlRW50cnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...apache/pinot/common/lineage/LineageEntryState.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9MaW5lYWdlRW50cnlTdGF0ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...rg/apache/pinot/common/lineage/SegmentLineage.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/common/lineage/SegmentLineageUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZVV0aWxzLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ot/common/messages/RoutingTableRebuildMessage.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvUm91dGluZ1RhYmxlUmVidWlsZE1lc3NhZ2UuamF2YQ==) | `0.00% <ø> (ø)` | |
   | ... and [1102 more](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=footer). Last update [a8974c7...33ce463](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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] fx19880617 commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       Agreed, it's good to add logs to measure the startup time.




----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   ok PTAL I finished polishing the exit status work and updated Javadoc


----------------------------------------------------------------
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] fx19880617 merged pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   


----------------------------------------------------------------
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 a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       Add a log before starting the service and another one after the service is started and measure log the time it took to start

##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       I dont think there is any reason to not add controller. @fx19880617 




----------------------------------------------------------------
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] codecov-commenter commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #5917:
URL: https://github.com/apache/incubator-pinot/pull/5917#issuecomment-680784776


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=h1) Report
   > Merging [#5917](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `22.91%`.
   > The diff coverage is `48.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5917/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #5917       +/-   ##
   ===========================================
   - Coverage   66.44%   43.53%   -22.92%     
   ===========================================
     Files        1075     1184      +109     
     Lines       54773    62044     +7271     
     Branches     8168     9479     +1311     
   ===========================================
   - Hits        36396    27013     -9383     
   - Misses      15700    32596    +16896     
   + Partials     2677     2435      -242     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `43.53% <48.81%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `26.66% <0.00%> (-30.48%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [.../org/apache/pinot/common/lineage/LineageEntry.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9MaW5lYWdlRW50cnkuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...apache/pinot/common/lineage/LineageEntryState.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9MaW5lYWdlRW50cnlTdGF0ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...rg/apache/pinot/common/lineage/SegmentLineage.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/common/lineage/SegmentLineageUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZVV0aWxzLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ot/common/messages/RoutingTableRebuildMessage.java](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvUm91dGluZ1RhYmxlUmVidWlsZE1lc3NhZ2UuamF2YQ==) | `0.00% <ø> (ø)` | |
   | ... and [1104 more](https://codecov.io/gh/apache/incubator-pinot/pull/5917/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=footer). Last update [a8974c7...33ce463](https://codecov.io/gh/apache/incubator-pinot/pull/5917?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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] kishoreg commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       Ah nice idea. Go for it!

##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       Ah nice idea. 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] fx19880617 commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       If it's a new cluster, then controller has to be started first, otherwise broker and server will fail.
   
   Probably we can add a check to decide if the cluster is already there then start controller in parallel as well.




----------------------------------------------------------------
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 a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:
+        new Thread("Starting " + role) {
+          @Override public void run() {
+            startPinotService(role, properties);
+          }
+        }.start();
+        break;
+      default:
+        startPinotService(role, properties);

Review comment:
       Yes, each service. Also log the wall clock time, that will help us understand what’s debug slow starts




----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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






----------------------------------------------------------------
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] fx19880617 commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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






----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   one problem is the logging of `org.apache.helix.manager.zk.ZKHelixManager`. Trying the cluster clutters the log like this even when we catch exception:
   
   ```
   fail to createClient.
   org.apache.helix.HelixException: Cluster structure is not set up for cluster: QuickStartCluster
   	at org.apache.helix.manager.zk.ZKHelixManager.handleNewSession(ZKHelixManager.java:1124) ~[pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.helix.manager.zk.ZKHelixManager.createClient(ZKHelixManager.java:701) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.helix.manager.zk.ZKHelixManager.connect(ZKHelixManager.java:738) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.clusterExists(StartServiceManagerCommand.java:260) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.startBootstrapServices(StartServiceManagerCommand.java:228) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.execute(StartServiceManagerCommand.java:181) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartControllerCommand.execute(StartControllerCommand.java:130) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.QuickstartRunner.startControllers(QuickstartRunner.java:105) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.QuickstartRunner.startAll(QuickstartRunner.java:140) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.HybridQuickstart.execute(HybridQuickstart.java:132) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.HybridQuickstart.main(HybridQuickstart.java:57) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   fail to connect ServiceManager_localhost_-1
   org.apache.helix.HelixException: Cluster structure is not set up for cluster: QuickStartCluster
   	at org.apache.helix.manager.zk.ZKHelixManager.handleNewSession(ZKHelixManager.java:1124) ~[pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.helix.manager.zk.ZKHelixManager.createClient(ZKHelixManager.java:701) ~[pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.helix.manager.zk.ZKHelixManager.connect(ZKHelixManager.java:738) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.clusterExists(StartServiceManagerCommand.java:260) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.startBootstrapServices(StartServiceManagerCommand.java:228) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.execute(StartServiceManagerCommand.java:181) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.StartControllerCommand.execute(StartControllerCommand.java:130) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.QuickstartRunner.startControllers(QuickstartRunner.java:105) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.admin.command.QuickstartRunner.startAll(QuickstartRunner.java:140) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.HybridQuickstart.execute(HybridQuickstart.java:132) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   	at org.apache.pinot.tools.HybridQuickstart.main(HybridQuickstart.java:57) [pinot-all-0.5.0-SNAPSHOT-jar-with-dependencies.jar:0.5.0-SNAPSHOT-3508311f31a79d45a279d3a2fe1437bdfd2358be]
   ```
   
   I'm changing to always synchronously start CONTROLLER as it is better than scary logs


----------------------------------------------------------------
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] adriancole commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       hmm so in this case, if you have multiple CLI args, you can be in the wrong order subtly.. maybe they need to be sorted?




----------------------------------------------------------------
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] fx19880617 commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   Right, this PR will be included in next release(0.6.0)


----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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






----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   nah chicken, egg as servicemanager starts the roles. I think this is good as it gets.


----------------------------------------------------------------
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] adriancole commented on pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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


   I'm pretty satisfied with this now
   
   ```
   $ docker logs pinot 2>&1|grep 'INFO.*since launch'
   08:51:48,903 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [ServiceManager] at 0.02s since launch
   08:51:54,182 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [ServiceManager] instance [ServiceManager_55e6f54e58a8_7098] at 5.299s since launch
   08:51:54,368 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [CONTROLLER] at 5.485s since launch
   08:52:00,930 INFO  [main] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [CONTROLLER] instance [Controller_55e6f54e58a8_9000] at 12.047s since launch
   08:52:00,933 INFO  [Starting SERVER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [SERVER] at 12.05s since launch
   08:52:00,933 INFO  [Starting BROKER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:260) - Starting a Pinot [BROKER] at 12.05s since launch
   08:52:04,767 INFO  [Starting BROKER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [BROKER] instance [Broker_55e6f54e58a8_8099] at 15.885s since launch
   08:52:04,834 INFO  [Starting SERVER] command.StartServiceManagerCommand (StartServiceManagerCommand.java:262) - Started Pinot [SERVER] instance [Server_55e6f54e58a8_8098] at 15.952s since launch
   ```


----------------------------------------------------------------
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] fx19880617 commented on a change in pull request #5917: Starts Broker and Server in parallel when using ServiceManager

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



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartServiceManagerCommand.java
##########
@@ -192,7 +192,20 @@ public boolean execute()
   }
 
   private void startPinotService(Map<String, Object> properties) {
-    startPinotService(ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString()), properties);
+    ServiceRole role = ServiceRole.valueOf(properties.get(PINOT_SERVICE_ROLE).toString());
+    switch (role) {
+      // Broker and Server can be started in parallel always
+      case BROKER:
+      case SERVER:

Review comment:
       to check if a cluster is already there, you can create a spectator HelixManager by:
   ```
   HelixManager spectatorHelixManager = HelixManagerFactory.getZKHelixManager(_clusterName, instance_id, InstanceType.SPECTATOR, _zkServers);
   spectatorHelixManager.connect();
   spectatorHelixManager.disconnect();
   ``` 
   
   If it throws exception, then it means the cluster is not there.




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