[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: KUDU-2411. Add scripts to build binaries for testing use .. KUDU-2411. Add scripts to build binaries for testing use This script will build a jar file containing "relocatable" binaries for use with a minicluster. The idea is that the resulting jar will be uploaded to Maven Central whenever there is a release. There should be one artifact built on Linux (preferably an old OS, such as RHEL 6) and one on macOS, so that people can run integration tests against Kudu on either platform. The current approach is to dynamically link the dependencies instead of statically linking them to optimize for the total size of the resulting test artifact. With kudu-tserver, kudu-master, and kudu admin tool binaries all linking against the same shared objects, the size of the jar file to download and unpack is significantly reduced. There are a bunch of shared objects we have to ship regardless, such as dependencies from thirdparty and security libraries from the system. Other pieces not included in this patch are code to depend on, locate, and extract the resulting JAR file, as well as a refactor of the MiniCluster code to use the resulting bits to start up a Kudu cluster as part of a Java integration test. Some of that work is underway or has already been merged as part of KUDU-2411. Very basic testing of this patch has been done on both Linux and macOS. We will need to do more testing and validation as the integration work progresses. Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Reviewed-on: http://gerrit.cloudera.org:8080/11377 Reviewed-by: Alexey Serbin Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- A build-support/build_mini_cluster_binaries.sh A build-support/relocate_binaries_for_mini_cluster.py 2 files changed, 470 insertions(+), 0 deletions(-) Approvals: Alexey Serbin: Looks good to me, but someone else must approve Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: KUDU-2411. Add scripts to build binaries for testing use .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py File build-support/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@217 PS3, Line 217: : def copy_file(src, dest): : > To me it wasn't so much about performance but about more user-friendly flow Thanks for clarifying your thought process. Your point about erroring out before making any changes on disk is fair, but since in this case that would only entail creating a few output directories it's not very serious. Since you don't feel strongly about it then let's leave it as-is for now, because I'm not motivated enough by the tradeoffs (slight user friendliness improvement vs slight maintainability edge) to feel like it's worth another revision of this patch. -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 04 Jan 2019 23:14:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: KUDU-2411. Add scripts to build binaries for testing use .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py File build-support/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@217 PS3, Line 217: : def copy_file(src, dest): : > I'm not really concerned about this from a performance perspective because To me it wasn't so much about performance but about more user-friendly flow of the script (I alluded to this in the second sentence in my suggestion). As a user I like working with scripts that work like this: 1. Check a bunch of stuff up front without mutating any on-disk state, erroring out if something is missing. 2. Do all the work. This gives me confidence that if I see an error in step 1, none of my on-disk state has changed. In this script, that would mean moving check_for_chrpath() to just before prep_artifact_dirs(). All that said, I appreciate your point about keeping it close to where chrpath is actually used, and I don't feel strongly about it. -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 04 Jan 2019 22:31:03 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: KUDU-2411. Add scripts to build binaries for testing use .. Patch Set 4: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh File build-support/build_mini_cluster_binaries.sh: http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53 PS1, Line 53: ding with MacPorts. > I'm not Alexey but it would be good to at least note this limitation in a T SGTM: I like the idea of adding a TODO note. http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py File build-support/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@284 PS1, Line 284: runtime dependencies. > I added a TODO because I'm not exactly sure either way, but we can always t SGTM -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 04 Jan 2019 22:21:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: KUDU-2411. Add scripts to build binaries for testing use .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh File build-support/build_mini_cluster_binaries.sh: http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53 PS1, Line 53: ding with MacPorts. > SGTM: I like the idea of adding a TODO note. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 04 Jan 2019 22:21:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/11377 )
Change subject: KUDU-2411. Add scripts to build binaries for testing use
..
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@11
PS1, Line 11: whenver
> I think you missed this one.
Oops, yep I didn't notice this comment.
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12
PS1, Line 12: preferably an old OS, such as RHEL 6
> Could you note the rationale for dynamic linking over static linking in the
Done
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh
File build-support/build_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53
PS1, Line 53: /usr/local/opt/openssl
> I'm not Alexey but it would be good to at least note this limitation in a T
Done
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py
File build-support/relocate_binaries_for_mini_cluster.py:
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@217
PS3, Line 217: # Make sure we have the chrpath command available.
: check_for_command('chrpath')
:
> We're going to call chrpath() a lot; no need to do this with each invocatio
I'm not really concerned about this from a performance perspective because this
script runs in a couple of seconds, but to reduce the number of unnecessary
invocations of `which` I moved this up a level to the Linux-specific call site.
I think it's better to keep the check close to the call site in this case
because of the lack of a significant performance or fail-fast concern. Let me
know if you think there's a larger benefit to putting it at the top of the file
behind an if (LINUX) flag.
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@238
PS3, Line 238: target_src = os.path.join(config[BUILD_BIN_DIR], target)
: target_dst = os.path.join(config[ARTIFACT_BIN_DIR], target)
> relocate_deps() already did this; perhaps you could feed in these by argume
Done
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@245
PS3, Line 245: PAT_LINUX_LIB_EXCLUDE =
re.compile('(libpthread|libc|libstdc\+\+|librt|libdl|libgcc.*)\.so')
> This deserves to be globally defined to raise its visibility. Also, since i
I had considered putting it up above but wasn't sure if being more visible was
outweighed by being close to the usage. I think it's reasonable to put it at
the top, though, so it's one of the first things one sees when opening the file
since it may get modified periodically. I also like your idea about making it
an extended-syntax regex. Done.
--
To view, visit http://gerrit.cloudera.org:8080/11377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75
Gerrit-Change-Number: 11377
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Fri, 04 Jan 2019 22:12:57 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11377 to look at the new patch set (#4). Change subject: KUDU-2411. Add scripts to build binaries for testing use .. KUDU-2411. Add scripts to build binaries for testing use This script will build a jar file containing "relocatable" binaries for use with a minicluster. The idea is that the resulting jar will be uploaded to Maven Central whenever there is a release. There should be one artifact built on Linux (preferably an old OS, such as RHEL 6) and one on macOS, so that people can run integration tests against Kudu on either platform. The current approach is to dynamically link the dependencies instead of statically linking them to optimize for the total size of the resulting test artifact. With kudu-tserver, kudu-master, and kudu admin tool binaries all linking against the same shared objects, the size of the jar file to download and unpack is significantly reduced. There are a bunch of shared objects we have to ship regardless, such as dependencies from thirdparty and security libraries from the system. Other pieces not included in this patch are code to depend on, locate, and extract the resulting JAR file, as well as a refactor of the MiniCluster code to use the resulting bits to start up a Kudu cluster as part of a Java integration test. Some of that work is underway or has already been merged as part of KUDU-2411. Very basic testing of this patch has been done on both Linux and macOS. We will need to do more testing and validation as the integration work progresses. Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 --- A build-support/build_mini_cluster_binaries.sh A build-support/relocate_binaries_for_mini_cluster.py 2 files changed, 470 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/11377/4 -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11377 )
Change subject: KUDU-2411. Add scripts to build binaries for testing use
..
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@11
PS1, Line 11: whenver
> whenever
I think you missed this one.
http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12
PS1, Line 12: preferably an old OS, such as RHEL 6
> I tested building on EL6 and running on Ubuntu 16.04.
Could you note the rationale for dynamic linking over static linking in the
commit message?
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh
File build-support/build_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53
PS1, Line 53: /usr/local/opt/openssl
> I am only an occasional macOS user and my Mac dev box recently crashed hard
I'm not Alexey but it would be good to at least note this limitation in a TODO
if you're going to merge the code as-is.
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py
File build-support/relocate_binaries_for_mini_cluster.py:
http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@35
PS1, Line 35: import sys
> That lib doesn't look like it's part of the default Python distribution and
Agreed; I didn't realize sh wasn't a standard module.
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py
File build-support/relocate_binaries_for_mini_cluster.py:
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@217
PS3, Line 217: # Make sure we have the chrpath command available.
: check_for_command('chrpath')
:
We're going to call chrpath() a lot; no need to do this with each invocation.
Seems like this is better done up-front, before we do any real work.
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@238
PS3, Line 238: target_src = os.path.join(config[BUILD_BIN_DIR], target)
: target_dst = os.path.join(config[ARTIFACT_BIN_DIR], target)
relocate_deps() already did this; perhaps you could feed in these by argument
instead?
http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@245
PS3, Line 245: PAT_LINUX_LIB_EXCLUDE =
re.compile('(libpthread|libc|libstdc\+\+|librt|libdl|libgcc.*)\.so')
This deserves to be globally defined to raise its visibility. Also, since it
may evolve over time, it'd be good to put each library on its own line to
minimize the diff when the list changes.
--
To view, visit http://gerrit.cloudera.org:8080/11377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75
Gerrit-Change-Number: 11377
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Fri, 04 Jan 2019 19:11:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11377 to look at the new patch set (#3). Change subject: KUDU-2411. Add scripts to build binaries for testing use .. KUDU-2411. Add scripts to build binaries for testing use This script will build a jar file containing "relocatable" binaries for use with a minicluster. The idea is that the resulting jar will be uploaded to Maven Central whenver there is a release. There should be one artifact built on Linux (preferably an old OS, such as RHEL 6) and one on macOS, so that people can run integration tests against Kudu on either platform. Other pieces not included in this patch are code to depend on, locate, and extract the resulting JAR file, as well as a refactor of the MiniCluster code to use the resulting bits to start up a Kudu cluster as part of a Java integration test. Some of that work is underway or has already been merged as part of KUDU-2411. Very basic testing of this patch has been done on both Linux and macOS. We will need to do more testing and validation as the integration work progresses. Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 --- A build-support/build_mini_cluster_binaries.sh A build-support/relocate_binaries_for_mini_cluster.py 2 files changed, 468 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/11377/3 -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2411. Add scripts to build binaries for testing use
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11377 to look at the new patch set (#2). Change subject: KUDU-2411. Add scripts to build binaries for testing use .. KUDU-2411. Add scripts to build binaries for testing use This script will build a jar file containing "relocatable" binaries for use with a minicluster. The idea is that the resulting jar will be uploaded to Maven Central whenver there is a release. There should be one artifact built on Linux (preferably an old OS, such as RHEL 6) and one on macOS, so that people can run integration tests against Kudu on either platform. Other pieces not included in this patch are code to depend on, locate, and extract the resulting JAR file, as well as a refactor of the MiniCluster code to use the resulting bits to start up a Kudu cluster as part of a Java integration test. Some of that work is underway or has already been merged as part of KUDU-2411. Very basic testing of this patch has been done on both Linux and macOS. We will need to do more testing and validation as the integration work progresses. Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 --- A build-support/build_mini_cluster_binaries.sh A build-support/relocate_binaries_for_mini_cluster.py 2 files changed, 446 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/11377/2 -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
