You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2021/11/23 16:52:24 UTC
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
zchovan@cloudera.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18049
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Fix readlink behaviour on MacOS in start_kudu.sh
The MacOS version of 'readlink' doesn't support -f option and throws an
illegal option error, e.g.
./start_kudu.sh -m 3 -t 3 -b $BUILD_DIR -c $CLUSTER_DIR --rpc-master 8800
readlink: illegal option -- f
usage: readlink [-n] [file ...]
Added a script that imitates the same behaviour when MacOS is detected.
Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
---
M src/kudu/scripts/start_kudu.sh
1 file changed, 22 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/18049/1
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <zc...@cloudera.com>
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
> Yup, indeed that seemed to be used just for informational purposes as of la
I think we would only need the realpath if we are running the script from a different directory (so not kudu/scripts/) and we do not provide a -b/--builddir parameter. In that case $BUILDDIR will be $PWD and WEBSERVER_DOC_ROOT, KUDUMASTER, KUDUTSERVER variable will point to the wrong place and the startup will fail with a "Cannot find webroot directory" error.
Maybe we could make this more clear in the help text for the -b param and remove the readlink line?
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Nov 2021 17:40:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
> I think we would only need the realpath if we are running the script from a
It seems it's assumed to run the script from the build directory if not providing -b/--builddir command-line option, right?
I think it would help if that assumption is explicitly stated in the help/usage output for this script.
BTW, the essential part of the script are the binaries of kudu-master and kudu-tserver, and webserver doc root isn't a prerequisite to run a Kudu mini-cluster, IIUC. In addition, kudu-master and kudu-tserver do automatically find the proper location of the docroot if KUDU_HOME environment variable is set.
With that, I'd probably try to explore the following updates to the script:
* remove the usage of realpath at all since everything is capable of running on symlinked paths, IIUC
* add a mention that the script if assumed to run from the build directory unless -b/--builddir option is specified
* set WEBSERVER_DOC_ROOT to $BUILDDIR/../../www only if $KUDU_HOME isn't defined/empty or $KUDU_HOME/www isn't available
* don't require the presence of the web server doc root: just issue a warning if it's not available either as $BUILDDIR/../../www or as $KUDU_HOME/www
* don't set the --webserver_doc_root flag for kudu-master and kudu-tserver if WEBSERVER_DOC_ROOT directory as defined above doesn't exit
Would it make sense?
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Nov 2021 20:19:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18049
to look at the new patch set (#2).
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Fix readlink behaviour on MacOS in start_kudu.sh
The MacOS version of 'readlink' doesn't support -f option and throws an
illegal option error, e.g.
./start_kudu.sh -m 3 -t 3 -b $BUILD_DIR -c $CLUSTER_DIR --rpc-master 8800
readlink: illegal option -- f
usage: readlink [-n] [file ...]
Added a script that imitates the same behaviour when MacOS is detected.
Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
---
M src/kudu/scripts/start_kudu.sh
1 file changed, 21 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/18049/2
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] start kudu.sh script changes
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: start_kudu.sh script changes
......................................................................
start_kudu.sh script changes
- removed the readlink -f line as it wasn't providing any value
- added check for detecting if $KUDU_HOME is set or $KUDU_HOME/www is
available
- WEBSERVER_DOC_ROOT was made optional, doesn't stop the startup if not
found
- webserver_doc_root flag is not set on master/tserver if not found
- added warning to clarify the usage of -b|--builddir flag
Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Reviewed-on: http://gerrit.cloudera.org:8080/18049
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/scripts/start_kudu.sh
1 file changed, 22 insertions(+), 6 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] start kudu.sh script changes
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: start_kudu.sh script changes
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Nov 2021 11:03:36 +0000
Gerrit-HasComments: No
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
> looks like this readlink never actually did anything useful, other than thr
Yup, indeed that seemed to be used just for informational purposes as of latest version.
Do we actually need to find realpath for the directory? Does it mean we can simply remove that informational output and make the script more portable?
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 24 Nov 2021 17:16:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
looks like this readlink never actually did anything useful, other than throw a warning on macos. It seems it originates here: https://gerrit.cloudera.org/c/14192/6/examples/quickstart/scripts/start_kudu.sh. While you're at it, can you make sure the result in both OSs is used as intended?
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 24 Nov 2021 14:57:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
> It seems it's assumed to run the script from the build directory if not pro
@Alexey
Yes, it makes sense, I've updated this change to reflect what you've described.
The only thing that is not clear to me is what happens when the --webserver_doc_root flag is not set for kudu-master and kudu-tserver. Doesn't that mean that the static pages can't load on the webui? In that case isn't the whole webui part kind of useless?
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Nov 2021 14:28:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] Fix readlink behaviour on MacOS in start kudu.sh
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: Fix readlink behaviour on MacOS in start_kudu.sh
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18049/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18049/2//COMMIT_MSG@16
PS2, Line 16: Added a script that imitates the same behaviour when MacOS is detected.
Update the commit message to reflect the current behavior.
http://gerrit.cloudera.org:8080/#/c/18049/2/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/2/src/kudu/scripts/start_kudu.sh@132
PS2, Line 132: echo "Cannot find webroot directory $WEBSERVER_DOC_ROOT at \$KUDU_HOME/www or \$BUILDDIR/../../www"
nit: too long
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Nov 2021 15:07:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] start kudu.sh script changes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: start_kudu.sh script changes
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh
File src/kudu/scripts/start_kudu.sh:
http://gerrit.cloudera.org:8080/#/c/18049/1/src/kudu/scripts/start_kudu.sh@78
PS1, Line 78: echo $(readlink -f $(dirname $0))
> @Alexey
According to the code in https://github.com/apache/kudu/blob/e266f7139983fd8316ec97f046db6125124c77de/src/kudu/server/webserver_options.cc#L59-L63 , the webserver's doc root is automatically pointed to $KUDU_HOME/www when the flag isn't set. As for the case when the the webserver's doc root directory doesn't exist at all, the webserver runs OK, but much of the functionality isn't available, right; however, that doesn't stop kudu-tserver or kudu-master process to successfully start (at least that how it looked like some time ago).
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Nov 2021 19:02:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] start kudu.sh script changes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18049 )
Change subject: start_kudu.sh script changes
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Nov 2021 03:23:53 +0000
Gerrit-HasComments: No
[kudu-CR] start kudu.sh script changes
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18049
to look at the new patch set (#3).
Change subject: start_kudu.sh script changes
......................................................................
start_kudu.sh script changes
- removed the readlink -f line as it wasn't providing any value
- added check for detecting if $KUDU_HOME is set or $KUDU_HOME/www is
available
- WEBSERVER_DOC_ROOT was made optional, doesn't stop the startup if not
found
- webserver_doc_root flag is not set on master/tserver if not found
- added warning to clarify the usage of -b|--builddir flag
Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
---
M src/kudu/scripts/start_kudu.sh
1 file changed, 22 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/18049/3
--
To view, visit http://gerrit.cloudera.org:8080/18049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0057a6d6668488001be8b4f74f46010e2620b46e
Gerrit-Change-Number: 18049
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>