[kudu-CR] [java] allow disabling TLS transportation from java client
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16887 to look at the new patch set (#4). Change subject: [java] allow disabling TLS transportation from java client .. [java] allow disabling TLS transportation from java client In a cluster which is running in intranet, sometimes security is less concerned than performance in case port is not exposed. As mentioned in [1], on JAVA8 the TLS cypher performance is poor. And to disable TLS from server side requires to reboot cluster, it worths a trade off for a working cluster. This commit introduces KUDU_CLIENT_DISABLE_TLS environment variable to allow disabling use TLS transportation in client side. [1] https://issues.apache.org/jira/browse/KUDU-3133 Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed --- M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java A java/kudu-client/src/main/java/org/apache/kudu/util/SystemEnvUtil.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java 4 files changed, 178 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/16887/4 -- To view, visit http://gerrit.cloudera.org:8080/16887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed Gerrit-Change-Number: 16887 Gerrit-PatchSet: 4 Gerrit-Owner: wangning <1994wangn...@gmail.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] allow disabling TLS transportation from java client
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16887 to look at the new patch set (#5). Change subject: [java] allow disabling TLS transportation from java client .. [java] allow disabling TLS transportation from java client In a cluster which is running in intranet, sometimes security is less concerned than performance in case port is not exposed. As mentioned in [1], on JAVA8 the TLS cypher performance is poor. And to disable TLS from server side requires to reboot cluster, it worths a trade off for a working cluster. This commit introduces KUDU_CLIENT_DISABLE_TLS environment variable to allow disabling use TLS transportation in client side. [1] https://issues.apache.org/jira/browse/KUDU-3133 Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed --- M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java A java/kudu-client/src/main/java/org/apache/kudu/util/SystemEnvUtil.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java 4 files changed, 179 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/16887/5 -- To view, visit http://gerrit.cloudera.org:8080/16887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed Gerrit-Change-Number: 16887 Gerrit-PatchSet: 5 Gerrit-Owner: wangning <1994wangn...@gmail.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] allow disabling TLS transportation from java client
wangning has posted comments on this change. (
http://gerrit.cloudera.org:8080/16887 )
Change subject: [java] allow disabling TLS transportation from java client
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16887/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java:
http://gerrit.cloudera.org:8080/#/c/16887/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@102
PS3, Line 102: static {
> Perhaps it's worth moving the methods in the reporter to a utility class fo
Done
http://gerrit.cloudera.org:8080/#/c/16887/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@102
PS3, Line 102:
> nit: rename into KUDU_CLIENT_DISABLE_TLS
Done
http://gerrit.cloudera.org:8080/#/c/16887/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@260
PS3, Line 260: }
> This sounds like a good idea, but TLS_AUTHENTICATION_ONLY should be present
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16887
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed
Gerrit-Change-Number: 16887
Gerrit-PatchSet: 5
Gerrit-Owner: wangning <1994wangn...@gmail.com>
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: wangning <1994wangn...@gmail.com>
Gerrit-Comment-Date: Wed, 23 Dec 2020 09:21:53 +
Gerrit-HasComments: Yes
[kudu-CR] [java] allow disabling TLS transportation from java client
wangning has posted comments on this change. ( http://gerrit.cloudera.org:8080/16887 ) Change subject: [java] allow disabling TLS transportation from java client .. Patch Set 5: > Patch Set 3: > > Should this also be mirrored in the C++ client for unified behavior? It seems I can do some cpp cipher/non-cipher transportation benchmark, to find if there is any performance promotion/cpu free. We also strongly relied on impala. -- To view, visit http://gerrit.cloudera.org:8080/16887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed Gerrit-Change-Number: 16887 Gerrit-PatchSet: 5 Gerrit-Owner: wangning <1994wangn...@gmail.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: wangning <1994wangn...@gmail.com> Gerrit-Comment-Date: Wed, 23 Dec 2020 09:53:13 + Gerrit-HasComments: No
[kudu-CR] [java] allow disabling TLS transportation from java client
wangning has posted comments on this change. ( http://gerrit.cloudera.org:8080/16887 ) Change subject: [java] allow disabling TLS transportation from java client .. Patch Set 5: > Patch Set 5: > > > Patch Set 3: > > > > Should this also be mirrored in the C++ client for unified behavior? > > It seems I can do some cpp cipher/non-cipher transportation benchmark, to > find if there is any performance promotion/cpu free. We also strongly relied > on impala. I attached some pics in slack, it seems that c++ client can also benefit from this. https://getkudu.slack.com/archives/C0CPXJ3CH/p1608189443378800 -- To view, visit http://gerrit.cloudera.org:8080/16887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I762b9d6ef9257a7853684dd0aef315e5c5bba8ed Gerrit-Change-Number: 16887 Gerrit-PatchSet: 5 Gerrit-Owner: wangning <1994wangn...@gmail.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: wangning <1994wangn...@gmail.com> Gerrit-Comment-Date: Wed, 23 Dec 2020 12:14:52 + Gerrit-HasComments: No
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16897 ) Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG@9 PS2, Line 9: rashes and corruption : which was reverted in 2.8.1 > nit: could you add a reference to the issue in this commit message? Andrew +1 -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 17:19:39 + Gerrit-HasComments: Yes
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16897 to look at the new patch set (#3). Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. [thirpdarty] Upgrade to gperftools 2.8.1 gperftools 2.8.0 had a new feature that lead to crashes and corruption which was reverted in 2.8.1. This patch upgrades to 2.8.1 to avoid any issues. One of the issues that is fixed via feature revert in 2.8.1 is https://github.com/gperftools/gperftools/issues/1204 Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc --- M thirdparty/download-thirdparty.sh D thirdparty/patches/gperftools-osx-arm64.patch M thirdparty/vars.sh 3 files changed, 1 insertion(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/16897/3 -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16897 ) Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG@9 PS2, Line 9: rashes and corruption : which was reverted in 2.8.1 > +1 Done http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG@9 PS2, Line 9: rashes and corruption : which was reverted in 2.8.1 > nit: could you add a reference to the issue in this commit message? Andrew Done -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 17:38:06 + Gerrit-HasComments: Yes
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16897 ) Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 17:38:40 + Gerrit-HasComments: No
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16897 ) Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16897/2//COMMIT_MSG@9 PS2, Line 9: rashes and corruption : which was reverted in 2.8.1 > Done Thank you! -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 17:49:01 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add support for openSUSE in the Docker build
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16885 to look at the new patch set (#3). Change subject: [docker] Add support for openSUSE in the Docker build .. [docker] Add support for openSUSE in the Docker build This patch adds support for openSUSE/leap:15 as a base image in the Docker build. This is useful for testing and validating Kudu usage on openSUSE/SLES. I fixed the logic in `get_os_tag` to handle the illegal `/` character in the resulting tag. This results in a cleaner tag such as `apache/kudu:latest-leap15`. Change-Id: I9c1ce282fda67db5043445a139c888bc74c44680 --- M docker/bootstrap-dev-env.sh M docker/bootstrap-java-env.sh M docker/bootstrap-python-env.sh M docker/bootstrap-runtime-env.sh M docker/docker-build.py 5 files changed, 113 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/16885/3 -- To view, visit http://gerrit.cloudera.org:8080/16885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c1ce282fda67db5043445a139c888bc74c44680 Gerrit-Change-Number: 16885 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [docker] Add support for openSUSE in the Docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16885 ) Change subject: [docker] Add support for openSUSE in the Docker build .. Patch Set 2: (20 comments) http://gerrit.cloudera.org:8080/#/c/16885/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16885/2//COMMIT_MSG@15 PS2, Line 15: > nit: remove space Done http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh File docker/bootstrap-dev-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@185 PS2, Line 185: zypper > Does it make sense to add --non-interactive option to mirror what's done fo I don't think we have a license issue given it builds with no problems. The same for interactive modes. I guess this is a case of YAGNI. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@185 PS2, Line 185: update > According to https://en.opensuse.org/images/1/17/Zypper-cheat-sheet-1.pdf update does a refresh and update and matches the behavior of the other OS variants. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@209 PS2, Line 209: openssl-devel > Why openssl isn't enough? This was taken from the install instructions: https://kudu.apache.org/docs/installation.html#sles_from_source http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@220 PS2, Line 220: exta > nit: extra Done http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@221 PS2, Line 221: tzdata equivalent package. > What doesn't work not having this? Could you mention that either in the com Done http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@231 PS2, Line 231: vim > The 'vim' package is already in the fist list, no? Done http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh File docker/bootstrap-java-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh@76 PS2, Line 76: update > refresh ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh@76 PS2, Line 76: zypper > Add --non-interactive ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh@79 PS2, Line 79: install > Add --non-interactive and --auto-agree-with-licenses ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh@82 PS2, Line 82: clean > Add --all ? for what? http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-python-env.sh File docker/bootstrap-python-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-python-env.sh@104 PS2, Line 104: zypper > here and below: add --non-interactive flag for zypper? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-python-env.sh@106 PS2, Line 106: update > refresh ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-python-env.sh@117 PS2, Line 117: clean > clean --all ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh File docker/bootstrap-runtime-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@86 PS2, Line 86: zypper > here and below: add --non-interactive ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@90 PS2, Line 90: install > add --auto-agree-with-licenses ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@95 PS2, Line 95: openssl > Should this be openssl-devel ? It doesn't appear to be required for runtime. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@99 PS2, Line 99: install > add --auto-agree-with-licenses ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@105 PS2, Line 105: clean > clean --all ? see my other comment on this. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/docker-build.py File docker/docker-build.py: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/docker-build.py@165 PS2, Line 165: if "/" in base: > nit: it would be nice to add an example for the 'base' which requires extra Done -- To view, visit http://gerrit.cloudera.org:8080/16885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c1ce282fda67db5043445a139c888bc74c44680 Gerrit-Change-Number: 16885 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 18:25:43 + Gerrit-HasComments: Yes
[kudu-CR] [thirpdarty] Upgrade to gperftools 2.8.1
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16897 ) Change subject: [thirpdarty] Upgrade to gperftools 2.8.1 .. [thirpdarty] Upgrade to gperftools 2.8.1 gperftools 2.8.0 had a new feature that lead to crashes and corruption which was reverted in 2.8.1. This patch upgrades to 2.8.1 to avoid any issues. One of the issues that is fixed via feature revert in 2.8.1 is https://github.com/gperftools/gperftools/issues/1204 Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Reviewed-on: http://gerrit.cloudera.org:8080/16897 Reviewed-by: Bankim Bhavsar Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- M thirdparty/download-thirdparty.sh D thirdparty/patches/gperftools-osx-arm64.patch M thirdparty/vars.sh 3 files changed, 1 insertion(+), 23 deletions(-) Approvals: Bankim Bhavsar: Looks good to me, approved Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/16897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I69f3405d14c4a853d8c224b8111fef5961ea34dc Gerrit-Change-Number: 16897 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP KUDU-2612 Java client transaction API
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Hao Hao, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16894 to look at the new patch set (#2). Change subject: WIP KUDU-2612 Java client transaction API .. WIP KUDU-2612 Java client transaction API This is a raw WIP patch which at this point is more focused on the API than the actual functionality under the hood. The actual functionality to do the heavy-lifting (i.e. issuing RPC calls to TxnManager, retrying in case of transient errors, etc.) is not yet here, but I'm working on it. The proposed API is mirroring the API for the C++ client with a few twists because of the presence of the async-style objects in the Kudu Java client API. WIP: * get initial feedback on the proposed API: the semantics, signatures of the methods, supportability, extensibility, etc. * add actual functionality for the under-the-hood work * add more tests * change the commit description DONT_BUILD Change-Id: Idbb18e1ac0454a8ef9e3486430dfaa336e381e07 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java A java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java A java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransactionSerializer.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java 6 files changed, 544 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16894/2 -- To view, visit http://gerrit.cloudera.org:8080/16894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idbb18e1ac0454a8ef9e3486430dfaa336e381e07 Gerrit-Change-Number: 16894 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 ) Change subject: [rpc] weak_ptr for Messenger in RpcRetrier .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12 PS1, Line 12: retries > In general, yes. However, while doing so, it might try to access some fiel Sorry, a bit confused here, is the dist test you linked for this patch? If so, it seems it introduced some flakiness? -- To view, visit http://gerrit.cloudera.org:8080/16866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283 Gerrit-Change-Number: 16866 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 19:50:09 + Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 ) Change subject: [rpc] weak_ptr for Messenger in RpcRetrier .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12 PS1, Line 12: retries > Sorry, a bit confused here, is the dist test you linked for this patch? If The dist-test run at http://dist-test.cloudera.org/job?job_id=jenkins-slave.1607649291.2575691 was not produced by/with this patch. That one is referring to a run after e75aad09a8dbe295a11cc2042903b74faf72623e but before 49c922b5cc3e5d7ee885c6f355eec973a5334441. The latter fixed the issue I was referring to. This patch is more about dropping the reference to Messenger once the top-level object that created it is out of scope, so there would be needless retries if some RPC are still in-flight. Basically, that's about freeing unneeded resources earlier. -- To view, visit http://gerrit.cloudera.org:8080/16866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283 Gerrit-Change-Number: 16866 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 20:04:21 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add support for openSUSE in the Docker build
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16885 ) Change subject: [docker] Add support for openSUSE in the Docker build .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh File docker/bootstrap-dev-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@185 PS2, Line 185: update > update does a refresh and update and matches the behavior of the other OS v ack http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@185 PS2, Line 185: zypper > I don't think we have a license issue given it builds with no problems. The OK, but then I'm curious: why do we use DEBIAN_FRONTEND for Ubuntu/Debian which is similar to --non-interactive for zypper? http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-dev-env.sh@209 PS2, Line 209: openssl-devel > This was taken from the install instructions: https://kudu.apache.org/docs/ I see: I was under impression openssl was enough on SLES. Most likely I was wrong, but regardless we should keep this list and the list in the build instructions in sync, I agree. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh File docker/bootstrap-java-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-java-env.sh@82 PS2, Line 82: clean > for what? To remove repo metadata as well, I guess. As per https://en.opensuse.org/SDB:Zypper_manual_%28plain%29 clean (cc) [options] [alias|name|#|URI]... Clean the local caches for all known or specified repositories. By default, only caches of downloaded packages are cleaned. -m, --metadata Clean repository metadata cache instead of package cache. -M, --raw-metadata Clean repository raw metadata cache instead of package cache. -a, --all Clean both repository metadata and package caches. http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh File docker/bootstrap-runtime-env.sh: http://gerrit.cloudera.org:8080/#/c/16885/2/docker/bootstrap-runtime-env.sh@95 PS2, Line 95: openssl > It doesn't appear to be required for runtime. Ah, I see -- it seems I confused run-time and development images. Does krb5-devel is needed for runtime? -- To view, visit http://gerrit.cloudera.org:8080/16885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c1ce282fda67db5043445a139c888bc74c44680 Gerrit-Change-Number: 16885 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 20:22:21 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add support for openSUSE in the Docker build
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16885 ) Change subject: [docker] Add support for openSUSE in the Docker build .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c1ce282fda67db5043445a139c888bc74c44680 Gerrit-Change-Number: 16885 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Dec 2020 20:55:40 + Gerrit-HasComments: No
[kudu-CR] (wip) KUDU-2612: restrict TxnStatusManager calls to be made by the leader only
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16648 )
Change subject: (wip) KUDU-2612: restrict TxnStatusManager calls to be made by
the leader only
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/integration-tests/txn_status_table-itest.cc
File src/kudu/integration-tests/txn_status_table-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/integration-tests/txn_status_table-itest.cc@756
PS2, Line 756: ASSERT_OK(s);
: ASSERT_TRUE(txn_status.has_user());
: ASSERT_STREQ(kUser, txn_status.user().c_str());
: ASSERT_TRUE(txn_status.has_state());
: ASSERT_EQ(TxnStatePB::OPEN, txn_status.state());
Does this work as expected when an assertion triggers? I remember there were
issues with that, so in this sort of construction we tended to use CHECK_
equivalents.
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/integration-tests/txn_status_table-itest.cc@764
PS2, Line 764: std::for_each(threads.begin(), threads.end(), [&] (thread& t) {
t.join(); });
If leave those ASSERTs above (i.e. not converting them into CHECKs), this
should be put into a scoped cleanup, right? Otherwise, if an assertion above
triggers, the threads would not be joined?
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/master/txn_manager-test.cc@150
PS2, Line 150:
nit: remove the extra space
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/master/txn_manager-test.cc@151
PS2, Line 151:
cluster_->mini_tablet_server(0)->server()->tablet_manager()->GetTabletReplicas(&replicas)
I'm not sure this waits or assumes any leadership status, saw
GetTabletReplicas() simply calls AppendValuesFromMap() under the hood, no?
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/master/txn_manager-test.cc@161
PS2, Line 161: tablet_replica_
What tablet replica is it and where is it used besides TearDown() and Start()?
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.h@95
PS2, Line 95: txn_status
nit: txn_status_manager
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.cc@202
PS2, Line 202: const string& uuid =
txn_status_manager_->status_tablet_.tablet_replica_->permanent_uuid();
What happens if an instance of this lock is being constructed and the tablet
replica is reset upon calling
https://gerrit.cloudera.org/#/c/16648/2/src/kudu/tablet/tablet_replica.cc@211
TSTabletManager::CreateNewTablet() ?
This has triggered the following TSAN warnings:
WARNING: ThreadSanitizer: data race (pid=972)
Write of size 8 at 0x7b5400056548 by thread T152 (mutexes: write
M634861673190352280, write M58672
#0
std::__1::enable_if<(is_move_constructible::value) &&
(is_move_assigna
#1
std::__1::shared_ptr::swap(std::__1::shared_ptr&)
#2
std::__1::shared_ptr::operator=(std::__1::shared_ptr
const&,
#5
kudu::tserver::TSTabletManager::CreateNewTablet(std::__1::basic_string::__call::operator()() const
/data/1/hao/Repositories/kud
#11 std::__1::function::operator()() const
/data/1/hao/Repositories/kudu/thirdparty/ins
#12 kudu::ThreadPool::DispatchThread()
/data/1/hao/Repositories/kudu/src/kudu/util/threadpool.cc
#13 kudu::ThreadPool::CreateThread()::$_1::operator()() const
/data/1/hao/Repositories/kudu/src/
#14
decltype(std::__1::forward(fp)())
std::__1::__invoke
#15 void
std::__1::__invoke_void_return_wrapper::__call::operator()() const
/data/1/hao/Repositories/kud
#19 std::__1::function::operator()() const
/data/1/hao/Repositories/kudu/thirdparty/ins
#20 kudu::Thread::SuperviseThread(void*)
/data/1/hao/Repositories/kudu/src/kudu/util/thread.cc:6
Previous read of size 8 at 0x7b5400056548 by thread T150 (mutexes: read
M586447839757432840):
#0 std::__1::shared_ptr::operator->() const
/data/1/hao/Repositories/kudu/
#1 kudu::tablet::TabletReplica::permanent_uuid() const
/data/1/hao/Repositories/kudu/src/kudu/ta
#2
kudu::transactions::TxnStatusManager::ScopedLeaderSharedLock::ScopedLeaderSharedLock(kudu::tr
#3 kudu::tserver::TSTabletManager::TxnStalenessTrackerTask()
/data/1/hao/Repositories/kudu/src/k
#4 kudu::tserver::TSTabletManager::Init()::$_8::operator()() const
/data/1/hao/Repositories/kudu
#5
decltype(std::__1::forward(fp)())
std::__1::__i
#6 void
std::__1::__invoke_void_return_wrapper
[kudu-CR] (wip) KUDU-2612: restrict TxnStatusManager calls to be made by the leader only
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16648 )
Change subject: (wip) KUDU-2612: restrict TxnStatusManager calls to be made by
the leader only
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.cc@a482
PS2, Line 482:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
Can we at least check that we have acquired the leader lock?
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/transactions/txn_status_manager.cc@724
PS2, Line 724: auto* consensus = status_tablet_.tablet_replica_->consensus();
: DCHECK(consensus);
nit: can combine as:
auto* consensus = DCHECK_NOTNULL(...consensus());
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16648/2/src/kudu/tserver/ts_tablet_manager.cc@1420
PS2, Line 1420: {
: shared_lock l(lock_);
: for (const auto& elem : tablet_map_) {
: auto r = elem.second;
: // Find txn status tablet replicas.
: if (r->txn_coordinator()) {
: replicas.emplace_back(std::move(r));
: }
: }
: }
: for (auto& r : replicas) {
: if (shutdown_latch_.count() == 0) {
: return;
: }
: auto* coordinator = DCHECK_NOTNULL(r->txn_coordinator());
It may be worth considering updating this to look more like how we schedule
maintenance ops. E.g. rather than iterating through each replica and trying to
check for coordinator, instead when we create a replica that is a coordinator,
"register" that coordinator with the TSTabletManager when it's fully started
and ready.
Conceptually, the problems at hand seems similar, that we want to run tasks
using a shared threadpool, and that the tasks are tied to the our replicas, and
the replica shutting down shouldn't collide with the task running. Take a look
at util/maintenance_manager.cc to see if a similar approach might make sense
here, e.g. when shutting down a replica, if an abort task is in progress, we
wait for it to complete, and then proceed with the shutdown. That might also
help avoid races with this task and the initial call to TabletReplica::Start().
--
To view, visit http://gerrit.cloudera.org:8080/16648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1ad095dcb4bdffcbe0ecf9631a60bac208c2a
Gerrit-Change-Number: 16648
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Dec 2020 00:08:26 +
Gerrit-HasComments: Yes
