[kudu-CR] KUDU-2973 Refactor table names in catalog manager
Hello Tidy Bot, Kudu Jenkins, Anonymous Coward (314), Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15018 to look at the new patch set (#8). Change subject: KUDU-2973 Refactor table names in catalog manager .. KUDU-2973 Refactor table names in catalog manager The table names are "normalized" if HMS integration is enabled so that they can be synchronized to HMS. Hive's table/database identifiers are pretty restrictive (alphanumerical ASCII characters + underscore and forward slash) and it's case-insensitive. As Kudu doesn't support databases it imposes a further restriction: the table identifiers to be synchronized with HMS have to contain exactly one "." character which separates the database name from the table name in HMS. Catalog manager used to normalize the table name when HMS integration is enabled and use that for authorization among other things. Ranger's restrictions regarding identifiers are more lax, can be case sensitive and is not limited to ASCII. This commit introduces a method to parse database and table name for Ranger from a table identifier and refactors catalog manager to use the original table name for authorization instead of the normalized one and push the normalization to Sentry's implementation instead. Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 --- M CMakeLists.txt M src/kudu/common/table_util-test.cc M src/kudu/common/table_util.cc M src/kudu/common/table_util.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/sentry_authz_provider.cc A src/kudu/ranger/CMakeLists.txt A src/kudu/ranger/ranger_client.cc A src/kudu/ranger/ranger_client.h 11 files changed, 211 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/15018/8 -- To view, visit http://gerrit.cloudera.org:8080/15018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 Gerrit-Change-Number: 15018 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] webserver: use faster gzip compression
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15059 )
Change subject: webserver: use faster gzip compression
..
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15059/1/src/kudu/util/zlib.cc
File src/kudu/util/zlib.cc:
http://gerrit.cloudera.org:8080/#/c/15059/1/src/kudu/util/zlib.cc@76
PS1, Line 76: Status CompressLevel(Slice input, int level, ostream* out) {
Could DCHECK that level is between 1 and 9. Not a huge deal though as
deflateInit2() will return Z_STREAM_ERROR if it can't parse the level.
--
To view, visit http://gerrit.cloudera.org:8080/15059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If97d883483a7313bd5496af68b17e968a5223b1a
Gerrit-Change-Number: 15059
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 17 Jan 2020 07:12:35 +
Gerrit-HasComments: Yes
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15058 )
Change subject: build: restrict clang version, prefer lld, enable thinlto
..
Patch Set 1:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@239
PS1, Line 239: " CC=${THIRDPARTY_TOOLCHAIN_DIR}/bin/clang CXX=$CC++
cmake ")
May want to recommend the ccache wrappers in build-support/ccache-clang instead.
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@410
PS1, Line 410: if (NOT "${KUDU_USE_LTO}" STREQUAL "")
I think you can also get away with:
if (KUDU_USE_LTO)
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@416
PS1, Line 416: message(FATAL_ERROR "LTO only supported for release builds")
Could you explain why in a comment?
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@419
PS1, Line 419: set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}
-Wl,--thinlto-cache-dir=${PROJECT_BINARY_DIR}/lto.cache")
You sure you want PROJECT_BINARY_DIR? We don't call project() so what actually
gets used?
Elsewhere we would use CMAKE_CURRENT_BINARY_DIR for this sort of thing.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake
File cmake_modules/KuduLinker.cmake:
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@20
PS1, Line 20: execute_process(COMMAND which ld.gold
: OUTPUT_VARIABLE GOLD_LOCATION
: OUTPUT_STRIP_TRAILING_WHITESPACE
: ERROR_QUIET)
Want to move this down to where it's actually needed?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@25
PS1, Line 25: foreach(candidate_linker lld
"${THIRDPARTY_TOOLCHAIN_DIR}/bin/ld.lld" gold default)
May want to add a comment explaining the rationale for the ordering (i.e.
preferences).
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@26
PS1, Line 26: if(candidate_linker STREQUAL "default")
: set(candidate_flag "")
: else()
: set(candidate_flag "-fuse-ld=${candidate_linker}")
: endif()
Could you get away with this?
if(NOT candidate_linker STREQUAL "default")
set(candidate_flag "-fuse-ld=${candidate_linker}")
endif()
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@56
PS1, Line 56: This seems to be fixed in devtoolset-6 or later.
How was it fixed?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@80
PS1, Line 80: )
remove
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@85
PS1, Line 85: message(STATUS "Checking linker version with cflags: ${ARGN}")
Maybe adjust the message a bit if ${ARGN} is empty (i.e. the default linker)?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@89
PS1, Line 89: # We're expecting LINKER_OUTPUT to look like one of these:
: # GNU gold (version 2.24) 1.11
: # GNU gold (GNU Binutils for Ubuntu 2.30) 1.15
Nit: move this down to just above L93.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@102
PS1, Line 102: string(REGEX MATCH "GNU ld version (([0-9]+\\.?)+)" _
"${LINKER_OUTPUT}")
Would be great if you could provide some sample text to help illustrate how the
regex works for ld.bfd and lld.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@118
PS1, Line 118: set(LINKER_FOUND TRUEPARENT_SCOPE)
Typo here (I think you meant to omit PARENT_SCOPE?) Surprised it didn't give
you any problems.
--
To view, visit http://gerrit.cloudera.org:8080/15058
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0
Gerrit-Change-Number: 15058
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 17 Jan 2020 07:10:13 +
Gerrit-HasComments: Yes
[kudu-CR] webserver: use faster gzip compression
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15059 to review the following change. Change subject: webserver: use faster gzip compression .. webserver: use faster gzip compression If an HTTP client indicates that it supports gzip, we gzip-compress the web response. This can be beneficial over slow links, but the default gzip compression level is pretty slow. This changes the gzip compression to use the fastest level, which is several times faster, without that much loss in space. Change-Id: If97d883483a7313bd5496af68b17e968a5223b1a --- M src/kudu/server/webserver.cc M src/kudu/util/zlib.cc M src/kudu/util/zlib.h 3 files changed, 10 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/15059/1 -- To view, visit http://gerrit.cloudera.org:8080/15059 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If97d883483a7313bd5496af68b17e968a5223b1a Gerrit-Change-Number: 15059 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15058 to review the following change. Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 168 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/15058/1 -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15032 ) Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out The timeout handling code in Connection::HandleOutboundCallTimeout assumes that when a timeout is triggered the OutboundCall must still be valid. In the case of an asynchronous rpc that was cancelled before the timeout was hit, this will not be the case. The fix is to check if the OutboundCall is still valid and return without processing the timeout if it is not. Testing: - Added timeout tests to TestCancellation to inject timeouts at OutboundCall states from READY to SENT. - Tested in Impala in the context of implementing IMPALA-8712. Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Reviewed-on: http://gerrit.cloudera.org:8080/15032 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/rpc/connection.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc 3 files changed, 22 insertions(+), 9 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 ) Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc@380 PS1, Line 380: The RPC may have been cancelled before the timeout was hit > Done Thank you! -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 17 Jan 2020 01:27:41 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 ) Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 17 Jan 2020 01:25:20 + Gerrit-HasComments: No
[kudu-CR] [metrics] fix compilation on macOS
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. [metrics] fix compilation on macOS This is a follow-up to 926bca85827e597f2923652c3b3b017c7c071f3b. Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Reviewed-on: http://gerrit.cloudera.org:8080/15055 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/master/table_metrics.h M src/kudu/tablet/tablet_metrics.h M src/kudu/util/metrics.h 3 files changed, 2 insertions(+), 7 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15034 )
Change subject: KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on
server side
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h@152
PS4, Line 152: std::vector&& bfs,
> I understand but using rvalue reference as type for bfs forces caller to pa
I see. That runs afoul of the Google Style Guide's rules on rvalue references
though: https://google.github.io/styleguide/cppguide.html#Rvalue_references
If you grep across the Kudu codebase, all of the rvalue reference argument
types are either for move constructors, move assignment operators, or perfect
forwarding.
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/common/wire_protocol.cc@633
PS5, Line 633: if (!bf_src.has_log_space_bytes() ||
!bf_src.has_bloom_data() ||
: !bf_src.has_hash_algorithm() ||
bf_src.hash_algorithm() == UNKNOWN_HASH ||
: !bf_src.has_always_false()) {
: return Status::InvalidArgument("Invalid in bloom
filter predicate on column: "
: "missing bloom filter
details", col.name());
: }
Should this be moved into BlockBloomFilter::InitFromPB?
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/CMakeLists.txt@21
PS5, Line 21:
Can you fix the indentation for the continuation lines? Check out how the other
PROTOBUF_GENERATE_CPP do it in this file.
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/block_bloom_filter.cc@113
PS5, Line 113: memcpy(directory_, bf_src.bloom_data().data(),
bf_src.bloom_data().size());
Shouldn't we just do this copy to faithfully respect the state of bf_src,
regardless of the value of always_false?
--
To view, visit http://gerrit.cloudera.org:8080/15034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ecfd67e9c5fbe459c5b4aed91e0be2a194d433a
Gerrit-Change-Number: 15034
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: ZhangYao
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Fri, 17 Jan 2020 00:31:31 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Fix print function usage in Python 2
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15054 ) Change subject: [build] Fix print function usage in Python 2 .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/15054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e Gerrit-Change-Number: 15054 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 17 Jan 2020 00:28:10 + Gerrit-HasComments: No
[kudu-CR] [client] support resolve one master address to multiple addresses
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15036 )
Change subject: [client] support resolve one master address to multiple
addresses
..
Patch Set 3:
(5 comments)
In the case of a multi-homed deployment, our DNS resolution will yield multiple
addresses for the same machine. Do the fanned out RPCs respond OK if there's
more than one "I am the leader!" response?
I think it does (though the Java client will LOG about it); just want to double
check that assumption with you.
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java:
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@57
PS3, Line 57: private int numMasters;
Move this below the private final stuff, since it's now mutable.
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@83
PS3, Line 83: this.numMasters = masterAddrs.size();
Seems like we can avoid doing this now.
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@190
PS3, Line 190: d.addCallbacks(callbackForNode(hostAndPort),
errbackForNode(hostAndPort));
: deferreds.add(d);
To avoid duplicating this code, you could add to masterAddrsWithNames in this
error case too, but use an InetAddress of null. Then when you loop over
masterAddrsWithNames on L196, if the address is null you LOG and build the
deferred using Deferred.fromError, while if it's not null you call
connectToMaster to build the deferred.
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@103
PS3, Line 103: if (getAllInetAddresses(host) != null) {
Rewrite so you don't have to do the resolution multiple times:
InetAddress[] addrs = getAllInetAddresses(host);
if (addrs != null) {
return addrs[0];
}
return null;
http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@104
PS3, Line 104: return getAllInetAddresses(host)[0];
> Nit: Is a non-null but empty array a possibility? Better to check for that
Also, are we guaranteed that this:
InetAddress.getByName(...)
Is exactly equivalent to this:
InetAddress.getAllByName(...)[0]
Will they always yield the same result?
--
To view, visit http://gerrit.cloudera.org:8080/15036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a
Gerrit-Change-Number: 15036
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang
Gerrit-Comment-Date: Fri, 17 Jan 2020 00:22:42 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side
Hello Kudu Jenkins, helifu, Yao Xu, Adar Dembo, ZhangYao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15034 to look at the new patch set (#5). Change subject: KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side .. KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side This change switches the implementation of the ColumnPredicate to use the BlockBloomFilter for the BloomFilter predicate on the server side. Earlier implementation was still experimental and didn't provide public client APIs that actually use this BloomFilter predicate so taken the liberty to make incompatible wire protocol changes. Most of the change involves refactoring the implementation including the unit tests. Currently only FAST_HASH algorithm is supported since 32-bit versions of MURMUR2 and CITY_HASH are not currently implemented. Change-Id: I7ecfd67e9c5fbe459c5b4aed91e0be2a194d433a --- M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/column_predicate.h M src/kudu/common/common.proto M src/kudu/common/wire_protocol-test.cc M src/kudu/common/wire_protocol.cc M src/kudu/tablet/cfile_set-test.cc M src/kudu/util/CMakeLists.txt M src/kudu/util/block_bloom_filter.cc M src/kudu/util/block_bloom_filter.h A src/kudu/util/block_bloom_filter.proto M src/kudu/util/hash.proto M src/kudu/util/hash_util.h 13 files changed, 476 insertions(+), 314 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/15034/5 -- To view, visit http://gerrit.cloudera.org:8080/15034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7ecfd67e9c5fbe459c5b4aed91e0be2a194d433a Gerrit-Change-Number: 15034 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: ZhangYao Gerrit-Reviewer: helifu
[kudu-CR] KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/15034 )
Change subject: KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on
server side
..
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h@152
PS4, Line 152: std::vector&& bfs,
> Here and below you don't need the rvalue reference; could just pass std::ve
I understand but using rvalue reference as type for bfs forces caller to pass
rvalue(using std::move or directly) and avoid the costly copy of a vector by
mistake. Hence this is deliberate.
http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.h
File src/kudu/util/block_bloom_filter.h:
http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.h@78
PS3, Line 78: // Initialize the internal data structures using the supplied
arguments.
: // Useful for de-serializing the BlockBloomFilter.
: Status Init(int log_space_bytes, const void* src_data, size_t
src_len, bool always_false);
> Good point.
Thanks for the tip. Done.
http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.h@121
PS3, Line 121: // Returns amount of space used in log2 bytes.
: int GetLogSpaceBytes() const {
: return log_num_buckets_ + kLogBucketByteSize;
: }
> Same as above.
Done
http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.cc@108
PS3, Line 108:
> Ths Init variant calls InitInternal (memset on L91) and then calls memcpy (
Intent was to move the memset() outside of InitInternal() but missed removing
that line from InitInternal(). Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/15034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ecfd67e9c5fbe459c5b4aed91e0be2a194d433a
Gerrit-Change-Number: 15034
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: ZhangYao
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Fri, 17 Jan 2020 00:08:51 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2973 Refactor table names in catalog manager
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15018 )
Change subject: KUDU-2973 Refactor table names in catalog manager
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util-test.cc
File src/kudu/common/table_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util-test.cc@71
PS7, Line 71: ASSERT_OK(ParseRangerTableIdentifier(table, , ));
You can use EXPECT_OK in lieu of ASSERT_OK if you want the rest of the test to
continue running after the failure.
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util.h@40
PS7, Line 40: Status ParseRangerTableIdentifier(const std::string& table_name,
Should doc.
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util.cc
File src/kudu/common/table_util.cc:
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/common/table_util.cc@97
PS7, Line 97: if (ranger_table->empty() || ranger_database->empty()) {
Are these conditions actually possible given the code in L85-95? I don't see
how.
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15018/7/src/kudu/master/sentry_authz_provider.cc@348
PS7, Line 348: string normalized_ident =
CatalogManager::NormalizeTableName(table_ident);
If this normalization is now Sentry only, could you move this method into
sentry_authz_provider?
--
To view, visit http://gerrit.cloudera.org:8080/15018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5
Gerrit-Change-Number: 15018
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 17 Jan 2020 00:07:47 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] fix compilation on macOS
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 17 Jan 2020 00:01:44 + Gerrit-HasComments: No
[kudu-CR] [java] KUDU-2971: process communicates via protobuf-based protocol
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 ) Change subject: [java] KUDU-2971: process communicates via protobuf-based protocol .. Patch Set 10: (13 comments) http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@39 PS10, Line 39: MessageProcessor nit: there isn't a whole lot of processing being done in this class. It seems squarely focused on reading from the pipe and writing to the pipe. Maybe rename this MessageIO or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@130 PS10, Line 130:T bytesToMessage(byte[] data, Parser parser) Can this be static? Why is this in this class? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@50 PS10, Line 50: blockingQueue nit: since we're expecting there to eventually be an outbound queue, maybe call this inboundQueue or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@46 PS10, Line 46: blockingQueue nit: same here, maybe rename this to inboundQueue? Especially if the MessageWriter will interact with the outboundQueue. http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@48 PS10, Line 48: private ProtocolProcessor protocolProcessor; nit: not really sure what protocol is referring to here. Maybe call this RequestHandler requestHandler? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@107 PS10, Line 107: messageProcessor.bytesToMessage( : data, SubprocessRequestPB.parser()); This doesn't use any of the MessageProcessor's internal state, so maybe just inline the call here? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@44 PS10, Line 44: Subprocess.SubprocessResponsePB processRequest(Subprocess.SubprocessRequestPB request) nit: maybe call this handleSubprocessRequest() or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@58 PS10, Line 58: Responds nit: this doesn't actually send a response; maybe rename it and update this to focus on the fact that it's doing stuff with the request? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@63 PS10, Line 63: responses nit: maybe call this handleRequest() or something? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@40 PS10, Line 40: THREAD nit: this should be plural http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProcessor.java@60 PS10, Line 60: byte[] malformedMessage = "malformed message".getBytes(StandardCharsets.UTF_8); This doesn't look like it's creating a message that is longer than 1024*1024 bytes. Mind commenting why this is malformed? http://gerrit.cloudera.org:8080/#/c/14329/10/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageReaderWriter.java File
[kudu-CR] [metrics] fix compilation on macOS
Hello Yingchun Lai, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15055 to look at the new patch set (#2). Change subject: [metrics] fix compilation on macOS .. [metrics] fix compilation on macOS This is a follow-up to 926bca85827e597f2923652c3b3b017c7c071f3b. Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 --- M src/kudu/master/table_metrics.h M src/kudu/tablet/tablet_metrics.h M src/kudu/util/metrics.h 3 files changed, 2 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/15055/2 -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] [java] KUDU-2971: process communicates via protobuf-based protocol
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 ) Change subject: [java] KUDU-2971: process communicates via protobuf-based protocol .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/14329/8/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/14329/8/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@79 PS8, Line 79: > The problem is in the test the input stream is short, the reader task can g have you tried that? I think EOF has to be specifically sent to get an EOFException. If you don't close the stream or send EOF it will just return the bytes read and unblock. So you should be able to interrupt the thread, then write something to the stream and it would guarantee that it is interrupted when blockingQueue.put() is reached. -- To view, visit http://gerrit.cloudera.org:8080/14329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf9ad24dbc9acc681284b6433836271b5b4c7982 Gerrit-Change-Number: 14329 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 16 Jan 2020 23:24:49 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2973 Refactor table names in catalog manager
Hello Tidy Bot, Kudu Jenkins, Anonymous Coward (314), Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15018 to look at the new patch set (#7). Change subject: KUDU-2973 Refactor table names in catalog manager .. KUDU-2973 Refactor table names in catalog manager The table names are "normalized" if HMS integration is enabled so that they can be synchronized to HMS. Hive's table/database identifiers are pretty restrictive (alphanumerical ASCII characters + underscore and forward slash) and it's case-insensitive. As Kudu doesn't support databases it imposes a further restriction: the table identifiers to be synchronized with HMS have to contain exactly one "." character which separates the database name from the table name in HMS. Catalog manager used to normalize the table name when HMS integration is enabled and use that for authorization among other things. Ranger's restrictions regarding identifiers are more lax, can be case sensitive and is not limited to ASCII. This commit introduces a method to parse database and table name for Ranger from a table identifier and refactors catalog manager to use the original table name for authorization instead of the normalized one and push the normalization to Sentry's implementation instead. Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 --- M CMakeLists.txt M src/kudu/common/table_util-test.cc M src/kudu/common/table_util.cc M src/kudu/common/table_util.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/sentry_authz_provider.cc A src/kudu/ranger/CMakeLists.txt A src/kudu/ranger/ranger_client.cc A src/kudu/ranger/ranger_client.h 11 files changed, 209 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/15018/7 -- To view, visit http://gerrit.cloudera.org:8080/15018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 Gerrit-Change-Number: 15018 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 ) Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h@536 PS2, Line 536: static void DoTestExpectTimeout(const Proxy& p, > warning: method 'DoTestExpectTimeout' can be made static [readability-conve Done http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h@558 PS2, Line 558: int elapsed_millis = static_cast(sw.elapsed().wall_millis()); > warning: narrowing conversion from 'double' to 'int' [bugprone-narrowing-co Done -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 23:15:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15032 to look at the new patch set (#3). Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out The timeout handling code in Connection::HandleOutboundCallTimeout assumes that when a timeout is triggered the OutboundCall must still be valid. In the case of an asynchronous rpc that was cancelled before the timeout was hit, this will not be the case. The fix is to check if the OutboundCall is still valid and return without processing the timeout if it is not. Testing: - Added timeout tests to TestCancellation to inject timeouts at OutboundCall states from READY to SENT. - Tested in Impala in the context of implementing IMPALA-8712. Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca --- M src/kudu/rpc/connection.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc 3 files changed, 22 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/15032/3 -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [metrics] fix compilation on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. Patch Set 1: > Build Successful > > http://jenkins.kudu.apache.org/job/kudu-gerrit/20167/ : SUCCESS It's funny, but there are also various callbacks that also have size_t/uint64_t mixed, e.g. 'size_t TabletReplica::OnDiskSize()'. So, removing the custom instantiation for macOS breaks. Another alternative to fix the compilation breakage is making instances of atomic gauges like 'scoped_refptr> tablet_active_scanners' to be of 'size_t' type. And yet another is going through mixed instantiations, replacing uint64_t with size_t, where appropriate. Just an extra option to those mentioned in 879cd9ab7e55f70c00851e45b6682870bab9f4b1. -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 23:07:44 + Gerrit-HasComments: No
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 ) Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc@380 PS1, Line 380: The RPC may have been cancelled before the timeout was hit > Good find! Done -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 22:56:23 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15032 to look at the new patch set (#2). Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out .. KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out The timeout handling code in Connection::HandleOutboundCallTimeout assumes that when a timeout is triggered the OutboundCall must still be valid. In the case of an asynchronous rpc that was cancelled before the timeout was hit, this will not be the case. The fix is to check if the OutboundCall is still valid and return without processing the timeout if it is not. Testing: - Added timeout tests to TestCancellation to inject timeouts at OutboundCall states from READY to SENT. - Tested in Impala in the context of implementing IMPALA-8712. Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca --- M src/kudu/rpc/connection.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc 3 files changed, 18 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/15032/2 -- To view, visit http://gerrit.cloudera.org:8080/15032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca Gerrit-Change-Number: 15032 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [metrics] fix compilation on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. Patch Set 1: > > Why doesn't this code in metrics.h address this? > > > > #if defined(__APPLE__) > > #define METRIC_DEFINE_gauge_size(entity, name, label, unit, desc, > > level, ...) \ > > ::kudu::GaugePrototype METRIC_##name( > \ > > ::kudu::MetricPrototype::CtorArgs(#entity, #name, label, unit, > > desc, level, ## __VA_ARGS__)) > > #define METRIC_DECLARE_gauge_size(name) \ > > extern ::kudu::GaugePrototype METRIC_##name > > #else > > #define METRIC_DEFINE_gauge_size METRIC_DEFINE_gauge_uint64 > > #define METRIC_DECLARE_gauge_size METRIC_DECLARE_gauge_uint64 > > #endif > > Because on macOS size_t isn't uint64_t. From the other side, maybe it's better to remove the custom specialization for macOS. -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 22:44:10 + Gerrit-HasComments: No
[kudu-CR] [build] Fix print function usage in Python 2
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15054 ) Change subject: [build] Fix print function usage in Python 2 .. [build] Fix print function usage in Python 2 This fixes the scripts that use the Python 3 print function when running in Python 2. Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e Reviewed-on: http://gerrit.cloudera.org:8080/15054 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M build-support/clang_tidy_gerrit.py M build-support/iwyu.py M src/kudu/scripts/graph-metrics.py 3 files changed, 8 insertions(+), 0 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/15054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e Gerrit-Change-Number: 15054 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [metrics] fix compilation on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. Patch Set 1: > Why doesn't this code in metrics.h address this? > > #if defined(__APPLE__) > #define METRIC_DEFINE_gauge_size(entity, name, label, unit, desc, > level, ...) \ > ::kudu::GaugePrototype METRIC_##name(\ > ::kudu::MetricPrototype::CtorArgs(#entity, #name, label, unit, > desc, level, ## __VA_ARGS__)) > #define METRIC_DECLARE_gauge_size(name) \ > extern ::kudu::GaugePrototype METRIC_##name > #else > #define METRIC_DEFINE_gauge_size METRIC_DEFINE_gauge_uint64 > #define METRIC_DECLARE_gauge_size METRIC_DECLARE_gauge_uint64 > #endif Because on macOS size_t isn't uint64_t. -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 22:38:29 + Gerrit-HasComments: No
[kudu-CR] [metrics] fix compilation on macOS
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15055 ) Change subject: [metrics] fix compilation on macOS .. Patch Set 1: Why doesn't this code in metrics.h address this? #if defined(__APPLE__) #define METRIC_DEFINE_gauge_size(entity, name, label, unit, desc, level, ...) \ ::kudu::GaugePrototype METRIC_##name(\ ::kudu::MetricPrototype::CtorArgs(#entity, #name, label, unit, desc, level, ## __VA_ARGS__)) #define METRIC_DECLARE_gauge_size(name) \ extern ::kudu::GaugePrototype METRIC_##name #else #define METRIC_DEFINE_gauge_size METRIC_DEFINE_gauge_uint64 #define METRIC_DECLARE_gauge_size METRIC_DECLARE_gauge_uint64 #endif -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 22:30:38 + Gerrit-HasComments: No
[kudu-CR] [metrics] fix compilation on macOS
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15055 Change subject: [metrics] fix compilation on macOS .. [metrics] fix compilation on macOS This is a follow-up to 926bca85827e597f2923652c3b3b017c7c071f3b. Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 --- M src/kudu/master/table_metrics.cc M src/kudu/server/server_base.cc M src/kudu/tablet/tablet_metrics.cc 3 files changed, 3 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/15055/1 -- To view, visit http://gerrit.cloudera.org:8080/15055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8e7109aebec268db1d545e657c991e7791f85210 Gerrit-Change-Number: 15055 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] cfile: change BlockBuilder API to yield a vector of Slices
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15042 ) Change subject: cfile: change BlockBuilder API to yield a vector of Slices .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc@85 PS1, Line 85: data_slices.insert(data_slices.begin(), Slice(buffer_)); : *slices = std::move(data_slices); > I don't think reserve() can necessarily help because data_builder_->Finish( Yep, reserve() could help in the alternative approach that I suggested, but not here. I think it's not a big deal anyway, especially given the way how std::vector grows internally (i.e. reserve() might help only in certain cases on 2x size boundaries or alike). http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc@127 PS1, Line 127: contiguous_buf_ > that's true. I think adding that CHECK will break things, though, since thi SGMT -- To view, visit http://gerrit.cloudera.org:8080/15042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc7a5f148a4a43cedac2428f4c1a18d0f93a10db Gerrit-Change-Number: 15042 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 22:27:52 + Gerrit-HasComments: Yes
[kudu-CR] [build] Fix print function usage in Python 2
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15054 ) Change subject: [build] Fix print function usage in Python 2 .. Patch Set 1: Code-Review+2 If it unbreaks stuff, Ship It. -- To view, visit http://gerrit.cloudera.org:8080/15054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e Gerrit-Change-Number: 15054 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 22:21:24 + Gerrit-HasComments: No
[kudu-CR] [build] Fix print function usage in Python 2
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15054 Change subject: [build] Fix print function usage in Python 2 .. [build] Fix print function usage in Python 2 This fixes the scripts that use the Python 3 print function when running in Python 2. Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e --- M build-support/clang_tidy_gerrit.py M build-support/iwyu.py M src/kudu/scripts/graph-metrics.py 3 files changed, 8 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/15054/1 -- To view, visit http://gerrit.cloudera.org:8080/15054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iee528e66fc017180e50b796f8cc1d2ee31be8a5e Gerrit-Change-Number: 15054 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-2973 Refactor table names in catalog manager
Hello Tidy Bot, Kudu Jenkins, Anonymous Coward (314), Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15018 to look at the new patch set (#6). Change subject: KUDU-2973 Refactor table names in catalog manager .. KUDU-2973 Refactor table names in catalog manager The table names are "normalized" if HMS integration is enabled so that they can be synchronized to HMS. Hive's table/database identifiers are pretty restrictive (alphanumerical ASCII characters + underscore and forward slash) and it's case-insensitive. As Kudu doesn't support databases it imposes a further restriction: the table identifiers to be synchronized with HMS have to contain exactly one "." character which separates the database name from the table name in HMS. Catalog manager used to normalize the table name when HMS integration is enabled and use that for authorization among other things. Ranger's restrictions regarding identifiers are more lax, can be case sensitive and is not limited to ASCII. This commit introduces a method to parse database and table name for Ranger from a table identifier and refactors catalog manager to use the original table name for authorization instead of the normalized one and push the normalization to Sentry's implementation instead. Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 --- M CMakeLists.txt M src/kudu/common/table_util-test.cc M src/kudu/common/table_util.cc M src/kudu/common/table_util.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/sentry_authz_provider.cc A src/kudu/ranger/CMakeLists.txt A src/kudu/ranger/ranger_client.cc A src/kudu/ranger/ranger_client.h 11 files changed, 206 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/15018/6 -- To view, visit http://gerrit.cloudera.org:8080/15018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 Gerrit-Change-Number: 15018 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] memrowset: small optimizations for scanning
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14874 ) Change subject: memrowset: small optimizations for scanning .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/14874/2/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/14874/2/src/kudu/tablet/cbtree-test.cc@45 PS2, Line 45: using std::unique_ptr; nit: flip order -- To view, visit http://gerrit.cloudera.org:8080/14874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia44b34606439625fbbbcc83e3652455a8894a0b3 Gerrit-Change-Number: 14874 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 21:52:51 + Gerrit-HasComments: Yes
[kudu-CR] cfile: change BlockBuilder API to yield a vector of Slices
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15042 ) Change subject: cfile: change BlockBuilder API to yield a vector of Slices .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc@85 PS1, Line 85: vector data_slices; : data_builder_->Finish(ordinal_pos > yep, it seems the same since and it's called just once here. at least, mayb I don't think reserve() can necessarily help because data_builder_->Finish() will overwrite (move on top of) data_slices. http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc@127 PS1, Line 127: contiguous_buf_ > Ah, indeed. that's true. I think adding that CHECK will break things, though, since this gets called multiple times in the same test suite. I'll just add a comment that says that the slice is only valid until the next call to the same function -- To view, visit http://gerrit.cloudera.org:8080/15042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc7a5f148a4a43cedac2428f4c1a18d0f93a10db Gerrit-Change-Number: 15042 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 21:32:47 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: small optimizations for scanning
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14874 to look at the new patch set (#2). Change subject: memrowset: small optimizations for scanning .. memrowset: small optimizations for scanning This adds two small optimizations for MRS/CBTree. They should have a small (maybe not noticeable) effect on user scans, but hopefully will also improve the speed of flushing by a few percent, which I found to be CPU bound when using an NVM disk for storage. - add a new getter to fetch the stored values without also accessing the stored keys. In most cases we don't care about the encoded key when accessing MRS, so we can avoid potentially following the key pointer (should avoid a cache miss) Shows a small effect in the modified benchmark: I1210 10:04:46.209149 15145 cbtree-test.cc:795] Time spent Scan 400 keys 10 times (frozen): real 0.638s user 0.636s sys 0.000s I1210 10:04:46.804275 15145 cbtree-test.cc:806] Time spent Scan 400 keys 10 times (frozen, val-only): real 0.595s user 0.596s sys 0.000s ... though this one didn't show a major effect in memrowset-test on its own. - add prefetching of the MRS values during scanning Tested with: memrowset-test --gtest_filter=\*InsertCount\* --roundtrip_num_rows=1000 --gtest_repeat=10 t-test for the "all committed" result shows a significant (though <5%) effect: data: subset(d, V1 == "before")$V2 and subset(d, V1 == "after-prefetch")$V2 t = 3.4999, df = 18, p-value = 0.002557 alternative hypothesis: true difference in means is not equal to 0 95 percent confidence interval: 0.004876512 0.019523488 sample estimates: mean of x mean of y 0.36220.3500 Change-Id: Ia44b34606439625fbbbcc83e3652455a8894a0b3 --- M src/kudu/tablet/cbtree-test.cc M src/kudu/tablet/concurrent_btree.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h 4 files changed, 80 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/14874/2 -- To view, visit http://gerrit.cloudera.org:8080/14874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia44b34606439625fbbbcc83e3652455a8894a0b3 Gerrit-Change-Number: 14874 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: small optimizations for scanning
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/14874 ) Change subject: memrowset: small optimizations for scanning .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/14874/1/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/14874/1/src/kudu/tablet/cbtree-test.cc@754 PS1, Line 754: gscoped_ptr > iter( > Wanna modernize while you're here? Done http://gerrit.cloudera.org:8080/#/c/14874/1/src/kudu/tablet/cbtree-test.cc@797 PS1, Line 797: int64_t count = 0, total_len = 0; > warning: multiple declarations in a single statement reduces readability [r Done http://gerrit.cloudera.org:8080/#/c/14874/1/src/kudu/tablet/cbtree-test.cc@808 PS1, Line 808: int64_t count = 0, total_len = 0; > warning: multiple declarations in a single statement reduces readability [r Done -- To view, visit http://gerrit.cloudera.org:8080/14874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia44b34606439625fbbbcc83e3652455a8894a0b3 Gerrit-Change-Number: 14874 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 21:27:36 + Gerrit-HasComments: Yes
[kudu-CR] WIP [util] utilities to get info on cloud instance
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14866 ) Change subject: WIP [util] utilities to get info on cloud instance .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc File src/kudu/util/cloud/instance_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14866/1/src/kudu/util/cloud/instance_metadata.cc@86 PS1, Line 86: "169.254.169.123" > Sorry, I never saw IP address hardcoded. IMHO, it should be placed in textu We could place some configuration properties into a configuration file, but kudu-tserver and kudu-master don't have a configuration files as of now except, maybe, flagsfile. Probably, it would make sense to make this IP address and other properties configurable via run-time flags, but I don't think these are going to change ever till cloud providers support dedicated NTP servers. If the API used here is changed so it requires something to change in these would-be-configuration-properties, I expect _all_ this code should be rewritten. At least that's the impression I got while reading public documentation on the these topics. -- To view, visit http://gerrit.cloudera.org:8080/14866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083f4803decaef5d6e7d44184bbd98b074d2578b Gerrit-Change-Number: 14866 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Thu, 16 Jan 2020 21:20:56 + Gerrit-HasComments: Yes
[kudu-CR] tools: avoid extra 100ms sleep at end of table scan
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15040 ) Change subject: tools: avoid extra 100ms sleep at end of table scan .. tools: avoid extra 100ms sleep at end of table scan Prior to this patch, the table scan tool printed a status output every 5 seconds by starting a separate monitor thread. This thread's main loop would only check for the scan completion every 100ms, so after the scans completed, the process could hang for up to an extra 100ms before exiting. This changes the printing to happen from the main thread instead, with the waiting based on the actual scan completion. Now the tool exits immediately when the scan completes. Example output: Before: todd@turbo:~/kudu$ time ./build/latest/bin/kudu table scan localhost sequences | wc -l 79 real 0m0.180s user 0m0.041s sys 0m0.021s After: todd@turbo:~/kudu$ time ./build/latest/bin/kudu table scan localhost sequences | wc -l 79 real 0m0.078s user 0m0.055s sys 0m0.005s Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Reviewed-on: http://gerrit.cloudera.org:8080/15040 Reviewed-by: Bankim Bhavsar Reviewed-by: Grant Henke Tested-by: Grant Henke Reviewed-by: Alexey Serbin --- M src/kudu/tools/table_scanner.cc M src/kudu/tools/table_scanner.h 2 files changed, 6 insertions(+), 15 deletions(-) Approvals: Bankim Bhavsar: Looks good to me, but someone else must approve Grant Henke: Looks good to me, approved; Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Gerrit-Change-Number: 15040 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] tools: avoid extra 100ms sleep at end of table scan
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15040 ) Change subject: tools: avoid extra 100ms sleep at end of table scan .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Gerrit-Change-Number: 15040 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 20:51:11 + Gerrit-HasComments: No
[kudu-CR] tools: avoid extra 100ms sleep at end of table scan
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15040 ) Change subject: tools: avoid extra 100ms sleep at end of table scan .. Patch Set 2: Verified+1 Code-Review+2 The IWYU is known and fixed in a different patch. -- To view, visit http://gerrit.cloudera.org:8080/15040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Gerrit-Change-Number: 15040 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 20:20:40 + Gerrit-HasComments: No
[kudu-CR] tools: avoid extra 100ms sleep at end of table scan
Grant Henke has removed a vote on this change. Change subject: tools: avoid extra 100ms sleep at end of table scan .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/15040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Gerrit-Change-Number: 15040 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2973 Refactor table names in catalog manager
Hello Kudu Jenkins, Anonymous Coward (314), Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15018 to look at the new patch set (#5). Change subject: KUDU-2973 Refactor table names in catalog manager .. KUDU-2973 Refactor table names in catalog manager The table names are "normalized" if HMS integration is enabled so that they can be synchronized to HMS. Hive's table/database identifiers are pretty restrictive (alphanumerical ASCII characters + underscore and forward slash) and it's case-insensitive. As Kudu doesn't support databases it imposes a further restriction: the table identifiers to be synchronized with HMS have to contain exactly one "." character which separates the database name from the table name in HMS. Catalog manager used to normalize the table name when HMS integration is enabled and use that for authorization among other things. Ranger's restrictions regarding identifiers are more lax, can be case sensitive and is not limited to ASCII. This commit introduces a method to parse database and table name for Ranger from a table identifier and refactors catalog manager to use the original table name for authorization instead of the normalized one and push the normalization to Sentry's implementation instead. Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 --- M CMakeLists.txt M src/kudu/common/table_util-test.cc M src/kudu/common/table_util.cc M src/kudu/common/table_util.h M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/sentry_authz_provider.cc A src/kudu/ranger/CMakeLists.txt A src/kudu/ranger/ranger_client.cc A src/kudu/ranger/ranger_client.h 10 files changed, 205 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/15018/5 -- To view, visit http://gerrit.cloudera.org:8080/15018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I11431ff5bc75540edff56ef3d4ad384fa37d33d5 Gerrit-Change-Number: 15018 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] cfile: change BlockBuilder API to yield a vector of Slices
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15042 ) Change subject: cfile: change BlockBuilder API to yield a vector of Slices .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc@85 PS1, Line 85: vector data_slices; : data_builder_->Finish(ordinal_pos > isn't that equivalent from a performnace perspective? in either case, you'r yep, it seems the same since and it's called just once here. at least, maybe add appropriate data_slices.reserve()? http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/cfile_writer.cc@399 PS1, Line 399: ck(null_hea > I think given Slice is a simple value type there isn't any benefit to movin yup, indeed http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc@127 PS1, Line 127: contiguous_buf_ > nope, because the Slice doesn't own the pointed-to memory, so it has to out Ah, indeed. Does it mean the callers of this method should expect some strange results if calling this method twice in a row if they keep the returned results at the upper level? Maybe, it make sense to protect against that replacing contiguous_buf_.clear() with CHECK(contiguous_buf_.empty()) ? -- To view, visit http://gerrit.cloudera.org:8080/15042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc7a5f148a4a43cedac2428f4c1a18d0f93a10db Gerrit-Change-Number: 15042 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 18:28:46 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: clean up bitshuffle tests a little .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15043/2/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15043/2/src/kudu/cfile/encoding-test.cc@722 PS2, Line 722: v = min_val + rng.Uniform64(max_val - min_val); The ASAN failure seem legit: /home/jenkins-slave/workspace/kudu-master/1/src/kudu/cfile/encoding-test.cc:722:41: runtime error: signed integer overflow: 2147483647 - -2147483648 cannot be represented in type 'int' -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 18:05:13 + Gerrit-HasComments: Yes
[kudu-CR] [client] support resolve one master address to multiple addresses
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15036 ) Change subject: [client] support resolve one master address to multiple addresses .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java: http://gerrit.cloudera.org:8080/#/c/15036/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@104 PS3, Line 104: return getAllInetAddresses(host)[0]; Nit: Is a non-null but empty array a possibility? Better to check for that case as well. http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/client/client-internal.cc@687 PS3, Line 687: , and try to connect each to them. This needs rewording. ". Connecting to each one of them." http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/util/net/sockaddr.h@80 PS3, Line 80: of "of" should be removed http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/15036/3/src/kudu/util/net/sockaddr.cc@136 PS3, Line 136: addrs.size() Bug: This'll initialize addrs_str with addrs.size() empty strings and then further add same number of more strings with push_back() in the loop below. I think you are looking for addrs_str.reserve(addrs.size()) instead. -- To view, visit http://gerrit.cloudera.org:8080/15036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a Gerrit-Change-Number: 15036 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Comment-Date: Thu, 16 Jan 2020 18:01:09 + Gerrit-HasComments: Yes
[kudu-CR] [tserver] replace gscoped ptr with unique ptr in Scanner
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15048 ) Change subject: [tserver] replace gscoped_ptr with unique_ptr in Scanner .. Patch Set 1: Verified+1 (2 comments) Unrelated test failures: * KUDU-2335 * KUDU-3043 IWYU failure seems weird seems on my CentOS6 build server it passes with no issues: 2020-01-16 09:31:15,410 INFO: Checking 3 file(s)... IWYU would have edited 0 files on your behalf. Built target iwyu ve0518:scanner-spec.debug[scanner-spec]$ http://gerrit.cloudera.org:8080/#/c/15048/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/15048/1/src/kudu/tserver/tablet_service.cc@2190 PS1, Line 2190: lower_bound = val; > warning: Assigned value is garbage or undefined [clang-analyzer-core.uninit I'm going to ignore this garbage warning since the 'val' cannot be assigned a garbage value: ExtractPredicateValue() returns OK only if assigning a valid value to its output parameter. http://gerrit.cloudera.org:8080/#/c/15048/1/src/kudu/tserver/tablet_service.cc@2197 PS1, Line 2197: upper_bound = val; > warning: Assigned value is garbage or undefined [clang-analyzer-core.uninit ditto -- To view, visit http://gerrit.cloudera.org:8080/15048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieac533565f96773cc450de521703c21534020596 Gerrit-Change-Number: 15048 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 16 Jan 2020 17:58:29 + Gerrit-HasComments: Yes
[kudu-CR] [tserver] replace gscoped ptr with unique ptr in Scanner
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15048 ) Change subject: [tserver] replace gscoped_ptr with unique_ptr in Scanner .. [tserver] replace gscoped_ptr with unique_ptr in Scanner This is a small clean up to replace gscoped_ptr with std::unique_ptr and avoid extra call to operator new. I also moved the Scanner::IsInitialized() method into the private section. Change-Id: Ieac533565f96773cc450de521703c21534020596 Reviewed-on: http://gerrit.cloudera.org:8080/15048 Reviewed-by: Adar Dembo Tested-by: Alexey Serbin --- M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_service.cc 3 files changed, 39 insertions(+), 44 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Alexey Serbin: Verified -- To view, visit http://gerrit.cloudera.org:8080/15048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ieac533565f96773cc450de521703c21534020596 Gerrit-Change-Number: 15048 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [tserver] replace gscoped ptr with unique ptr in Scanner
Alexey Serbin has removed a vote on this change. Change subject: [tserver] replace gscoped_ptr with unique_ptr in Scanner .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/15048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ieac533565f96773cc450de521703c21534020596 Gerrit-Change-Number: 15048 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [build] Fix IWYU script
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15052 ) Change subject: [build] Fix IWYU script .. [build] Fix IWYU script This is a follow up to 965e59f to fix iwyu.py Change-Id: I8a6c05ad2a715887dd50e05df8fc83ed3cd4e72f Reviewed-on: http://gerrit.cloudera.org:8080/15052 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M build-support/iwyu.py 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8a6c05ad2a715887dd50e05df8fc83ed3cd4e72f Gerrit-Change-Number: 15052 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Fix IWYU script
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15052 ) Change subject: [build] Fix IWYU script .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a6c05ad2a715887dd50e05df8fc83ed3cd4e72f Gerrit-Change-Number: 15052 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 17:07:14 + Gerrit-HasComments: No
[kudu-CR] [build] Fix IWYU script
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15052 Change subject: [build] Fix IWYU script .. [build] Fix IWYU script This is a follow up to 965e59f to fix iwyu.py Change-Id: I8a6c05ad2a715887dd50e05df8fc83ed3cd4e72f --- M build-support/iwyu.py 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/15052/1 -- To view, visit http://gerrit.cloudera.org:8080/15052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8a6c05ad2a715887dd50e05df8fc83ed3cd4e72f Gerrit-Change-Number: 15052 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] tools: avoid extra 100ms sleep at end of table scan
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15040 ) Change subject: tools: avoid extra 100ms sleep at end of table scan .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/15040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7dcd8408b1dbd5322a2f5e8f4889f5f5660ece0 Gerrit-Change-Number: 15040 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 16 Jan 2020 15:55:39 + Gerrit-HasComments: No
[kudu-CR] [client] support resolve one master address to multiple addresses
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/15036 ) Change subject: [client] support resolve one master address to multiple addresses .. Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@57 PS1, Line 57: private int numMasters; > Based on the usage, doesn't seem like this needs to be a member variable. Done http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@144 PS1, Line 144:* @param masterAddresses the addresses of masters to fetch from > Too long, wrap. Done http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@182 PS1, Line 182: new Pair<>(addr, new HostAndPort(addr.getHostAddress(), hostAndPort.getPort(; > whitespace Done http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@203 PS1, Line 203: d.addCallbacks(callbackForNode(hostAndPort), errbackForNode(hostAndPort)); > +1; why should the "resolve all" behavior only happen for one master? Yeah, get all resolved addresses of each master seems more reasonable. http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@214 PS1, Line 214:* @return The callback object that can be added to the RPC request. > Nit: Better to combine the definition and assignment of "d" to line 219 bel Done http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java: http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@57 PS1, Line 57: > Javadoc the method. and change type to InetAddress[] This method is no longer needed. http://gerrit.cloudera.org:8080/#/c/15036/1/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@136 PS1, Line 136:* Given an InetAddress, checks to see if the address is a local address, by > Can the implementation be consolidated somewhat with getInetAddress? It's a Done -- To view, visit http://gerrit.cloudera.org:8080/15036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a Gerrit-Change-Number: 15036 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Comment-Date: Thu, 16 Jan 2020 11:44:47 + Gerrit-HasComments: Yes
[kudu-CR] [client] support resolve one master address to multiple addresses
Hello Kudu Jenkins, Adar Dembo, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15036 to look at the new patch set (#3). Change subject: [client] support resolve one master address to multiple addresses .. [client] support resolve one master address to multiple addresses The master addresses of kudu client are fixed during runtime, we need to restart the client if we want to change the master addresses. In some network settings we can map a domain name to multiple ip addresses, if we configure the client with such a domain name as a 'master address', resolve it and try to connect each ip address, we can easily change the master addresses by network settings modifications. Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java M src/kudu/client/client-internal.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h 6 files changed, 73 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/15036/3 -- To view, visit http://gerrit.cloudera.org:8080/15036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a Gerrit-Change-Number: 15036 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] support resolve one master address to multiple addresses
Hello Kudu Jenkins, Adar Dembo, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15036 to look at the new patch set (#2). Change subject: [client] support resolve one master address to multiple addresses .. [client] support resolve one master address to multiple addresses The master addresses of kudu client are fixed during runtime, we need to restart the client if we want to change the master addresses. In some network settings we can map a domain name to multiple ip addresses, if we configure the client with such a domain name as a 'master address', resolve it and try to connect each ip address, we can easily change the master addresses by network settings modifications. Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java M src/kudu/client/client-internal.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h 6 files changed, 73 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/15036/2 -- To view, visit http://gerrit.cloudera.org:8080/15036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie242bc1331596902fa16f1c6d1b439d78b73977a Gerrit-Change-Number: 15036 Gerrit-PatchSet: 2 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
