You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/23 20:05:51 UTC

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh 

Hello Alexey Serbin, Volodymyr Verovkin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/14535

to review the following change.


Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................

scripts: multi-master support for start_kudu.sh and stop_kudu.sh

Note: I changed the semantics of -m and -t as I think it's more common to
change the number of servers than the base of the port range.

Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
---
M src/kudu/scripts/start_kudu.sh
M src/kudu/scripts/stop_kudu.sh
2 files changed, 74 insertions(+), 40 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/14535/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/start_kudu.sh@31
PS1, Line 31: -h, --help         Print help
            : -m, --num-masters  Number of Kudu Master servers to start (default: 1)
            : -t, --num-tservers Number of Kudu Tablet Servers to start (default: 3)
            : --rpc-master       RPC port of first Kudu Master; HTTP port is the next number.
            :                    Subsequent Masters will have following numbers
            : --rpc-tserver      RPC port of first Kudu Tablet Server; HTTP port is the next
            :                    number. Subsequent Tablet Serv
> nit: align the description of the parameters?
Done


http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh
File src/kudu/scripts/stop_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh@25
PS1, Line 25: pkill -9 -e -u $(id -u) -x kudu-tserver
            : pkill -9 -e -u $(id -u) -x kudu-master
            : 
            : 
            : 
> If relying on pgrep, pkill comes along as well.  Maybe, simply do
Done


http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh@30
PS1, Line 30: 
            : 
            : 
            : 
            : 
> ditto:
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:11:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Volodymyr Verovkin, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14535

to look at the new patch set (#3).

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................

scripts: multi-master support for start_kudu.sh and stop_kudu.sh

Note: I changed the semantics of -m and -t as I think it's more common to
change the number of servers than the base of the port range.

Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
---
M src/kudu/scripts/start_kudu.sh
M src/kudu/scripts/stop_kudu.sh
2 files changed, 69 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/14535/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh
File src/kudu/scripts/stop_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh@25
PS2, Line 25: -e
It seems pkill on macOS and on CentOS6 doesn't recognize the '-e' option.



-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:21:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 1: Verified+1

Overriding Jenkins, Python package download failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 21:00:30 +0000
Gerrit-HasComments: No

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 1:

(3 comments)

few nits

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/start_kudu.sh@31
PS1, Line 31: -h, --help       help
            : -m, --num-masters Number of Kudu Master servers to start (default: 1)
            : -t, --num-tservers Number of Kudu Tablet Servers to start (default: 3)
            : --rpc-master     RPC port of first Kudu Master server (HTTP port is next number)
            : --rpc-tserver    RPC port of first Kudu Tablet Server (other servers
            :                  will have following numbers)
            : -b, --builddir   path to the Kudu build directory
nit: align the description of the parameters?


http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh
File src/kudu/scripts/stop_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh@25
PS1, Line 25: TSERVERS="$(pgrep -d ' ' kudu-tserver)"
            : if [ -n "$TSERVERS" ]; then
            :   echo "Killing tservers: $TSERVERS"
            :   kill -9 $TSERVERS
            : fi
If relying on pgrep, pkill comes along as well.  Maybe, simply do

  pkill -9 -u $(id -u) -x kudu-tserver

here?


http://gerrit.cloudera.org:8080/#/c/14535/1/src/kudu/scripts/stop_kudu.sh@30
PS1, Line 30: MASTERS="$(pgrep -d ' ' kudu-master)"
            : if [ -n "$MASTERS" ]; then
            :   echo "Killing masters: $MASTERS"
            :   kill -9 $MASTERS
            : fi
ditto:

  pkill -9 -u $(id -u) -x kudu-master



-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:06:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh
File src/kudu/scripts/stop_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh@25
PS2, Line 25: -u
> It seems pkill on macOS and on CentOS6 doesn't recognize the '-e' option.
Alright, no echoing I guess.



-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:33:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 3: Verified+1

Overriding Jenkins, this patch doesn't exercise any tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 03:45:29 +0000
Gerrit-HasComments: No

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed a vote on this change.

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh
File src/kudu/scripts/stop_kudu.sh:

http://gerrit.cloudera.org:8080/#/c/14535/2/src/kudu/scripts/stop_kudu.sh@25
PS2, Line 25: -u
> Alright, no echoing I guess.
Yup.  On macOS (and other BSD systems) there is '-l' option for pkill (a.k.a. long output):


  https://www.freebsd.org/cgi/man.cgi?query=pkill&apropos=0&sektion=0&manpath=FreeBSD+12.0-RELEASE+and+Ports&arch=default&format=html


Unfortunately, it's not available in pkill used in major Linux distros (at least it's not there on CentOS6).



-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 23:32:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14535 )

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................

scripts: multi-master support for start_kudu.sh and stop_kudu.sh

Note: I changed the semantics of -m and -t as I think it's more common to
change the number of servers than the base of the port range.

Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Reviewed-on: http://gerrit.cloudera.org:8080/14535
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/scripts/start_kudu.sh
M src/kudu/scripts/stop_kudu.sh
2 files changed, 69 insertions(+), 42 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Adar Dembo: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] scripts: multi-master support for start kudu.sh and stop kudu.sh

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Volodymyr Verovkin, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14535

to look at the new patch set (#2).

Change subject: scripts: multi-master support for start_kudu.sh and stop_kudu.sh
......................................................................

scripts: multi-master support for start_kudu.sh and stop_kudu.sh

Note: I changed the semantics of -m and -t as I think it's more common to
change the number of servers than the base of the port range.

Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
---
M src/kudu/scripts/start_kudu.sh
M src/kudu/scripts/stop_kudu.sh
2 files changed, 69 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/14535/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id8db19c9145b5c68bb437390e6c0608961680347
Gerrit-Change-Number: 14535
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>