[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has uploaded a new change for review. http://gerrit.cloudera.org:8080/7701 Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API with tablet copy, to avoid fsync() on each block during copying. Blocks are sync to disk together once the table copy is complete. I did a manual test to copy tablet with size of ~37GB. With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 --- M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 3 files changed, 27 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/7701/1 -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has abandoned this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/7700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I73822754fa3537535478bdd9a017e1cdf5b03e9a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#19). Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that for LBM container can be reused without flushing in-flight writable blocks to disk. This can bring down the log block container number and disk space consumption for first few flush/compaction operations when using log block manager. I performed manual test which creates a table with two tablets, 'kudu perf loadgen localhost: --num-rows-per-thread=2000 --num-threads=10 --keep-auto-table --table_num_buckets=2'. With this patch, 'log_block_manager_containers' dropped from 56 to 7. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 594 insertions(+), 310 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/19 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has uploaded a new patch set (#2). Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API with tablet copy, to avoid fsync() on each block during copying. Blocks are sync to disk together once the table copy is complete. I did a manual test to copy tablet with size of ~37GB. With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I73822754fa3537535478bdd9a017e1cdf5b03e9a --- M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 3 files changed, 27 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/7700/2 -- To view, visit http://gerrit.cloudera.org:8080/7700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I73822754fa3537535478bdd9a017e1cdf5b03e9a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has uploaded a new change for review. http://gerrit.cloudera.org:8080/7700 Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API with tablet copy, to avoid fsync() on each block during copying. Blocks are sync to disk together once the table copy is complete. I did a manual test to copy tablet with size of ~37GB. With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I73822754fa3537535478bdd9a017e1cdf5b03e9a --- A src/1943.patch A src/deletion.patch A src/inject_failure_test.patch A src/inject_fault.patch M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h A src/reorder.patch A src/tablet_copy.patch 9 files changed, 3,513 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/7700/1 -- To view, visit http://gerrit.cloudera.org:8080/7700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I73822754fa3537535478bdd9a017e1cdf5b03e9a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#18). Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that for LBM container can be reused without flushing in-flight writable blocks to disk. This can bring down the log block container number and disk space consumption for first few flush/compaction operations when using log block manager. I performed manual test which creates a table with two tablets, 'kudu perf loadgen localhost: --num-rows-per-thread=2000 --num-threads=10 --keep-auto-table --table_num_buckets=2'. With this patch, 'log_block_manager_containers' dropped from 56 to 7. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 591 insertions(+), 310 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/18 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Alexey Serbin has posted comments on this change. Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: (5 comments) some nits http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: PS2, Line 219: addrs.size() nit: while you are at it, would it make sense to update this to if (addrs.empty()) ? http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: PS2, Line 129: std::string* host nit: would it make sense to have the second parameter optional, i.e. change the signature to Status ResolveSockaddr(Sockaddr* addr, std::string* host = nullptr); ? http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/rpc/connection_id.h File src/kudu/rpc/connection_id.h: PS2, Line 67: nit: const? Or it's not feasible because of id0 = id1 assignments somewhere? If adding the 'const' modifier is feasible, consider adding it for other members as well. http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: PS2, Line 61: tserver::TabletServer::kDefaultPort, &addresses)); : generic_proxy_.reset(new server::GenericServiceProxy( : messenger_, addresses[0], host_port_.host())); : ts_proxy_.reset(new tserver::TabletServerServiceProxy( : messenger_, addresses[0], host_port_.host())); : consensus_proxy_.reset(new consensus::ConsensusServiceProxy( : messenger_, addresses[0], host_port_.host())); style nit: maybe, introduce variables (references) to address[0] and host_port_.host() and use those as arguments for 3 proxy constructors? http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: PS2, Line 110: string( nit: consider dropping this for brevity: return str; -- To view, visit http://gerrit.cloudera.org:8080/7687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 17: (10 comments) I think I'm done reviewing everything outside log_block_manager.cc. Still a lot to take in there. I've been making my way through it, but thought I'd publish my comments so far. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 892: TEST_P(TestCFileBothCacheTypes, TestReleaseBlock) { What happened to parameterizing this test for all block_time_to_flush values? In my earlier comment I suggested that doing so would be moot only if there was no block_time_to_flush-specific code in here. But there is such code, so we should do the parameterization so as to avoid bit-rot. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS17, Line 266: if (FLAGS_block_time_to_flush == "finalize") { : RETURN_NOT_OK(block_->Finalize()); : } else if (FLAGS_block_time_to_flush != "close" && : FLAGS_block_time_to_flush != "none") { : LOG(FATAL) << "Unknown value for block_time_to_flush: " :<< FLAGS_block_time_to_flush; : } This is incorrect. We should always Finalize() the block; whether or not we flush inside Finalize() is determined by the block manager (and the value of this gflag). http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: Line 43: DEFINE_string(block_time_to_flush, "finalize", > warning: redundant 'FLAGS_block_time_to_flush' declaration [readability-red Let's rename to "block_manager_flush_control": 1. "block_manager_" is the prefix we've been using for other BM-related flags. 2. "time_to_flush" sounds conspicuously like time to live and other such numerical values. Line 44: "When to flush blocks registered in a BlockTransaction. " This should apply to any WritableBlock, not just those in a BlockTransaction. PS17, Line 48: none Let's use "never" instead, since we're talking about a point in time. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 32: DECLARE_string(block_time_to_flush); This doesn't belong here (block_coalesce_close didn't belong either). http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: Line 340: RETURN_NOT_OK_HANDLE_ERROR(writer_->Flush(WritableFile::FLUSH_ASYNC)); This is where we should check the value of block_time_to_flush. We should only flush if it's 'finalize'. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1176: LOG(WARNING) << Substitute( I mentioned this to Dan in our impromptu meeting yesterday; perhaps you weren't yet there: we should remove this WARNING because with out-of-order block metadata on disk it'll fire more frequently. PS17, Line 1397: container_->block_manager() You can use 'lbm' here. Line 1410: RETURN_NOT_OK(container_->FlushData(block_offset_, block_length_)); And here we also need to gate on block_time_to_flush == 'finalize'. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [docs] Deprecate Java 7 and Spark 1
Grant Henke has uploaded a new change for review. http://gerrit.cloudera.org:8080/7699 Change subject: [docs] Deprecate Java 7 and Spark 1 .. [docs] Deprecate Java 7 and Spark 1 Starts the release notes for Kudu 1.5.0 and adds entries for Java 7 and Spark 1 deprecation. Change-Id: I050df790ca51729cc99866b9cd75933c3c907a4c --- M docs/prior_release_notes.adoc M docs/release_notes.adoc 2 files changed, 254 insertions(+), 189 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7699/1 -- To view, visit http://gerrit.cloudera.org:8080/7699 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I050df790ca51729cc99866b9cd75933c3c907a4c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke
[kudu-CR] [iwyu] update on the internal and boost mappings
Alexey Serbin has posted comments on this change. Change subject: [iwyu] update on the internal and boost mappings .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch File thirdparty/patches/llvm-iwyu-include-picker.patch: > even if the number of patches is the same? Oh, I see. That's the only way to refresh stale workspaces. All right, I'll bump the patch level up. -- To view, visit http://gerrit.cloudera.org:8080/7696 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Alexey Serbin has posted comments on this change. Change subject: [iwyu] update on the internal and boost mappings .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp File build-support/iwyu/mappings/boost-all.imp: Line 960: { include: ["", private, "", public ] }, > Are these also imported from upstream? Or your changes? These are my changes. OK, keeping them separate would be a better choice, agreed. From the other side, I could try to re-generate the whole file. http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch File thirdparty/patches/llvm-iwyu-include-picker.patch: > You need to bump LLVM's patchlevel for this to be incorporated in existing even if the number of patches is the same? -- To view, visit http://gerrit.cloudera.org:8080/7696 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Adar Dembo has posted comments on this change. Change subject: [iwyu] update on the internal and boost mappings .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/7696/1//COMMIT_MSG Commit Message: PS1, Line 11: IWUY IWYU http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp File build-support/iwyu/mappings/boost-all.imp: Line 960: { include: ["", private, "", public ] }, Are these also imported from upstream? Or your changes? If the latter, perhaps we should put them in a separate file so it's easier to apply updates from upstream. http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch File thirdparty/patches/llvm-iwyu-include-picker.patch: You need to bump LLVM's patchlevel for this to be incorporated in existing deployments. -- To view, visit http://gerrit.cloudera.org:8080/7696 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7696 Change subject: [iwyu] update on the internal and boost mappings .. [iwyu] update on the internal and boost mappings Updated the internal and boost mappings to avoid some bogus suggestions generated by the include-what-you-use tool. Also, shortened the list of files where IWUY suggestions are 'muted'. Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2 --- M build-support/iwyu/iwyu-filter.awk M build-support/iwyu/mappings/boost-all.imp M thirdparty/patches/llvm-iwyu-include-picker.patch 3 files changed, 21 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/1 -- To view, visit http://gerrit.cloudera.org:8080/7696 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] [iwyu] first pass
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4738 to look at the new patch set (#16). Change subject: [iwyu] first pass .. [iwyu] first pass Updated C++ source files in accordance with some of the recommendations from include-what-you-use (IWYU) tool: * remove unused header files * add missing header files * use forward declarations when possible As a result, time of compilation reduced ~10% if building with GNU make in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com: prior: real1m46.782s user47m1.633s sys 3m10.678s after: real1m36.867s user42m17.340s sys 2m54.117s The next step is automating the checks, so IWYU check would run automatically (like the LINT build). NOTE: As of now, not all recommendations from the tool are applied yet, especially for the test-related code. That's because some recommendations produced by IWYU are incorrect. Basically, every suggestion required manual inspection and judgment, at least it was necessary to verify the result code at least compiles. As a result, the process of applying those recommendations is tedious and time consuming. Hopefully, IWYU will get better in the future. Meanwhile, we can address the rest of the recommendations file-by-file or so in the short run. Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa --- M src/kudu/benchmarks/rle.cc M src/kudu/benchmarks/tpch/line_item_tsv_importer.h M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.h M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/benchmarks/wal_hiccup.cc M src/kudu/cfile/binary_dict_block.cc M src/kudu/cfile/binary_dict_block.h M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h M src/kudu/cfile/binary_prefix_block.cc M src/kudu/cfile/binary_prefix_block.h M src/kudu/cfile/block_cache-test.cc M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/block_compression.cc M src/kudu/cfile/block_compression.h M src/kudu/cfile/bloomfile-test-base.h M src/kudu/cfile/bloomfile-test.cc M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/bshuf_block.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.cc M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/index-test.cc M src/kudu/cfile/index_block.cc M src/kudu/cfile/index_block.h M src/kudu/cfile/index_btree.cc M src/kudu/cfile/index_btree.h M src/kudu/cfile/mt-bloomfile-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h M src/kudu/client/batcher.cc M src/kudu/client/batcher.h M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h M src/kudu/client/error-internal.cc M src/kudu/client/error-internal.h M src/kudu/client/error_collector.cc M src/kudu/client/error_collector.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/partitioner-internal.cc M src/kudu/client/partitioner-internal.h M src/kudu/client/predicate-test.cc M src/kudu/client/replica-internal.cc M src/kudu/client/replica-internal.h M src/kudu/client/resource_metrics.cc M src/kudu/client/resource_metrics.h M src/kudu/client/samples/sample.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scan_token-internal.h M src/kudu/client/scan_token-test.cc M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/session-internal.cc M src/kudu/client/session-internal.h M src/kudu/client/shared_ptr.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.cc M src/kudu/client/table_creator-internal.h M src/kudu/client/tablet-internal.h M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/client/write_op.cc M src/kudu/client/write_
[kudu-CR] KUDU-1942. Kerberos fails to log in on hostnames with capital letters
Alexey Serbin has posted comments on this change. Change subject: KUDU-1942. Kerberos fails to log in on hostnames with capital letters .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7693/2//COMMIT_MSG Commit Message: PS2, Line 12: With this change I was able to set up a working cluster on my laptop : with a capitalized hostname, where before it would fail as described in : the JIRA. Is it worth adding a small test to catch a regression, if any? http://gerrit.cloudera.org:8080/#/c/7693/2/src/kudu/security/init.cc File src/kudu/security/init.cc: PS2, Line 20: #include nit: maybe, #include ? -- To view, visit http://gerrit.cloudera.org:8080/7693 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7692 to look at the new patch set (#2). Change subject: KUDU-2032 (part 2): propagate master hostnames into client .. KUDU-2032 (part 2): propagate master hostnames into client This changes the client code to remember the user-specified master addresses and propagate them into the creation of master proxies. It's not possible to reproduce the necessary DNS configurations in a minicluster test, but with this patch I am now able to use 'kudu perf loadgen' against a Kerberized cluster even when my local krb5.conf has rdns=false. Change-Id: Ie5838f22c96f757124112b505825a53740468ce1 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/integration-tests/external_mini_cluster.cc 5 files changed, 56 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7692/2 -- To view, visit http://gerrit.cloudera.org:8080/7692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie5838f22c96f757124112b505825a53740468ce1 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] rpc: move ConnectionId to its own file
Todd Lipcon has submitted this change and it was merged. Change subject: rpc: move ConnectionId to its own file .. rpc: move ConnectionId to its own file This class was previously implemented inside of outbound_call.{cc,h} where it didn't quite belong. In the several years since the code was initially written we've moved more towards a "single class per header" model unless the classes are truly trivial or really tightly coupled. Neither is the case here. Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Reviewed-on: http://gerrit.cloudera.org:8080/7685 Reviewed-by: Alexey Serbin Tested-by: Todd Lipcon --- M src/kudu/rpc/CMakeLists.txt A src/kudu/rpc/connection_id.cc A src/kudu/rpc/connection_id.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/proxy.h M src/kudu/rpc/reactor.h M src/kudu/rpc/rpc_context.cc 8 files changed, 174 insertions(+), 120 deletions(-) Approvals: Todd Lipcon: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: move ConnectionId to its own file
Todd Lipcon has posted comments on this change. Change subject: rpc: move ConnectionId to its own file .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] gutil: remove use of deprecated headers
Todd Lipcon has submitted this change and it was merged. Change subject: gutil: remove use of deprecated headers .. gutil: remove use of deprecated headers We've been on C++11 for a long time now. This commit removes usage of ext/hash_map and ext/hash_set headers in favor of the now-standardized unordered_map and unordered_set. Additionally, it removes our specializations of __gnucxx::hash<> and instead changes to specializing std::hash<> where appropriate. Some of those specializations are now part of the standard and have been removed as necessary to fix compilation. This does not yet remove the -Wno-deprecated setting in the top-level CMakeLists.txt, since it appears we have a lot of usage of our own internal deprecated functions. Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Reviewed-on: http://gerrit.cloudera.org:8080/7684 Tested-by: Todd Lipcon Reviewed-by: Alexey Serbin --- M CMakeLists.txt M src/kudu/gutil/CMakeLists.txt M src/kudu/gutil/endian.h M src/kudu/gutil/hash/hash.cc M src/kudu/gutil/hash/hash.h M src/kudu/gutil/strings/serialize.cc M src/kudu/gutil/strings/serialize.h M src/kudu/gutil/strings/split.cc M src/kudu/gutil/strings/split.h 9 files changed, 40 insertions(+), 126 deletions(-) Approvals: Todd Lipcon: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] security: only lookup hostname if HOST substitution is required
Hello Sailesh Mukil, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7694 to review the following change. Change subject: security: only lookup hostname if _HOST substitution is required .. security: only lookup hostname if _HOST substitution is required The Kerberos principal configuration uses the special token '_HOST' to indicate that the FQDN of the host should be specified. Previously we would always lookup the FQDN even if the substitution was not required, which might mean that startup would fail if there was no FQDN available, even if no _HOST substitution was required. Now, we only lookup the FQDN if FLAGS_principal contains the substitution token. This provides the possibility of a workaround of explicit principal configuration on machines with no FQDN. Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe --- M src/kudu/security/init.cc 1 file changed, 10 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7694/1 -- To view, visit http://gerrit.cloudera.org:8080/7694 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] KUDU-1942. Kerberos fails to log in on hostnames with capital letters
Hello Sailesh Mukil, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7693 to review the following change. Change subject: KUDU-1942. Kerberos fails to log in on hostnames with capital letters .. KUDU-1942. Kerberos fails to log in on hostnames with capital letters This ensures that servers canonicalize their FQDNs to lower-case before generating Kerberos principal names. With this change I was able to set up a working cluster on my laptop with a capitalized hostname, where before it would fail as described in the JIRA. I also verified that I was able to connect from both C++ and Java clients. Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627 --- M src/kudu/security/init.cc 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7693/1 -- To view, visit http://gerrit.cloudera.org:8080/7693 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 254: // sidecars attached to the call as it may result in use-after-free of the sidecars. > per comment in header, I think it's worth explaining why this is reasonable Added some explanation in the header. Refer to it here. PS3, Line 273: outbound transfer for this call so references to the sidecars : // can be relinquished. > I think this is no longer necessary to be added since the explanation of th Done http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS3, Line 304: e.g. older version of Kudu server), it's fatal for the > it's not clear what behavior this is implying. If it will FATAL, I think we Done http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: Line 223: return sidecar_byte_size_ > 0; > this isn't quite accurate to what the description says. It is possible to h Right. Comments updated. Renamed function to HasNonEmptySidecars(). -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7599 to look at the new patch set (#4). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 532 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/4 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] gutil: remove use of deprecated headers
Alexey Serbin has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Adar Dembo has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Andrew Wong has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/7602/9/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 381: metadata_.swap(metadata); > Why is this (and the other changes to 'metadata' in this function) necessar Ah, I started to split out the behavior of FsManager::CreateInit.. or Open to be only called once. Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7602 to look at the new patch set (#10). Change subject: separate DataDirManager from BlockManagers .. separate DataDirManager from BlockManagers Currently, the DataDirManager is owned by the BlockManagers. Since only blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under the roots specified by 'fs_data_dirs'), this hierarchy made sense. However, it would be nice to track other files that fall within 'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the directory manager from the block manager makes this more feasible. This patch introduces a number of changes to the FsManager, BlockManager, and DataDirManager with respect to initialization. - BlockManagers no longer have a Create() function - BlockManager's dtor will now just wait for DataDir closures to finish instead of shutting down the DataDirManager as to not directly affect the DataDirManager - DataDirManagers now have two static constructors: one to open an existing layout, and another to create and open a new layout - FsManagers will only create the BlockManager when opening an fs layout - FsManagers will only Open() a DataDirManager if one has not already been constructed - A validator is added to check the value of FLAGS_block_manager before any of the above initialization can occur Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace --- M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/util/path_util.cc M src/kudu/util/path_util.h 16 files changed, 443 insertions(+), 306 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/10 -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] gutil: remove use of deprecated headers
Todd Lipcon has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 2: Verified+1 some java flake -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rpc: some small cleanup in ConnectionId
Alexey Serbin has posted comments on this change. Change subject: rpc: some small cleanup in ConnectionId .. Patch Set 2: Code-Review+2 It seems there were unrelated flaky tests in RELEASE and SAN configs, but overall LGTM. -- To view, visit http://gerrit.cloudera.org:8080/7686 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Adar Dembo has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/7602/9/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 381: metadata_.swap(metadata); Why is this (and the other changes to 'metadata' in this function) necessary? Why can't we just populate metadata_ in Open() as before? -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] rpc: move ConnectionId to its own file
Alexey Serbin has posted comments on this change. Change subject: rpc: move ConnectionId to its own file .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Andrew Wong has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS8, Line 144: : : : : : : : > Don't we need to preserve this in some capacity? Done http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS8, Line 231: qcquired > acquired Done http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 164 > Maybe restore this comment? Done Line 71: return value == "log" || value == "file"; > On macOS can you set this up so that "file" is the only valid option? Done Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_, > Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by Fs Yeah, it actually seems that this is mandatory. I was banging my head trying to figure out the test failures, and it seems like the double-initting was causing an early release of file locks. Anyway, the simple fix is to not run this block if dd_manager_ has already been constructed. A slightly more complicated fix might require changing the create/open of FsManager in a similar way, but I think that would belong in a separate patch. -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7602 to look at the new patch set (#9). Change subject: separate DataDirManager from BlockManagers .. separate DataDirManager from BlockManagers Currently, the DataDirManager is owned by the BlockManagers. Since only blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under the roots specified by 'fs_data_dirs'), this hierarchy made sense. However, it would be nice to track other files that fall within 'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the directory manager from the block manager makes this more feasible. This patch introduces a number of changes to the FsManager, BlockManager, and DataDirManager with respect to initialization. - BlockManagers no longer have a Create() function - BlockManager's dtor will now just wait for DataDir closures to finish instead of shutting down the DataDirManager as to not directly affect the DataDirManager - DataDirManagers now have two static constructors: one to open an existing layout, and another to create and open a new layout - FsManagers will only create the BlockManager when opening an fs layout - FsManagers will only Open() a DataDirManager if one has not already been constructed - A validator is added to check the value of FLAGS_block_manager before any of the above initialization can occur Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace --- M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/util/path_util.cc M src/kudu/util/path_util.h 17 files changed, 449 insertions(+), 311 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/9 -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client
Hello Dan Burkert, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7692 to review the following change. Change subject: KUDU-2032 (part 2): propagate master hostnames into client .. KUDU-2032 (part 2): propagate master hostnames into client This changes the client code to remember the user-specified master addresses and propagate them into the creation of master proxies. It's not possible to reproduce the necessary DNS configurations in a minicluster test, but with this patch I am now able to use 'kudu perf loadgen' against a Kerberized cluster even when my local krb5.conf has rdns=false. Change-Id: Ie5838f22c96f757124112b505825a53740468ce1 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/integration-tests/external_mini_cluster.cc 5 files changed, 50 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7692/1 -- To view, visit http://gerrit.cloudera.org:8080/7692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie5838f22c96f757124112b505825a53740468ce1 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 254: // attached to the call or it may result in use-after-free of the sidecars. per comment in header, I think it's worth explaining why this is reasonable PS3, Line 273: We assume that the remote supports footer or the call : // doesn't have any sidecars. I think this is no longer necessary to be added since the explanation of this assumption is in the CancelOutboundTransfer function itself. http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS3, Line 304: the assumption is that the call doesn't have any sidecar. it's not clear what behavior this is implying. If it will FATAL, I think we should be explicit here (and perhaps reproduce the reasoning from our gerrit discussion that we didn't start using outbound sidecars until the same version when we introduced footers, therefore it's not possible to have a call with sidecars but no footer support) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: Line 223: return sidecar_byte_size_ > 0; this isn't quite accurate to what the description says. It is possible to have an empty sidecar (I think we actually do this in the case of an empty scan result, for example) -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 16: > (5 comments) > > Hao, Dan, and I had a long discussion about this patch, and I > wanted to reproduce our conclusions here for everyone else: > > 1. The cfile and block performance-related knobs are useful for > testing, but are not as important as having a good API and a robust > implementation. IIUC, the resplitting of FlushDataAsync and > Finalize was due to retaining all the existing flags; let's not do > this. Let's stick to the earlier approach (where Finalize implies a > flush), and adjust the flags if need be. > > 2. What flags are worth preserving? We identified the need for just > one, which dictates when a block's data should be flushed. It has > three values: "finalize" (default value), "close" (defers the flush > to when the entire transaction is committed), and "none" (doesn't > flush at all, "flushing" will happen when the transaction commits > and files are fsynced). > > 3. When should AppendMetadata be called? One option is to do it at > Finalize time. Another is to defer it to Close. The problem with > doing it during Finalize is that it exacerbates the "1. AppendData, > 2. AppendMetadata, 3. SyncData, 4. SyncMetadata" race: if we crash > between #2 and #3, we may end up with just the metadata on disk, > and if we AppendMetadata during Finalize, we have many more blocks' > worth of metadata that can land on disk without corresponding data. > > 4. But AppendMetadata is called during Close, we may end up with > out-of-order metadata entries on disk, since transactions could > commit out of order. Do we care? We don't think so; the LBM > implementation should already be robust to this. An alternative is > to use an in-memory buffer to retain metadata order, but this > didn't seem worth the complexity. > > 5. What to do about zero length blocks? Should their metadata be > written, or skipped? The answer is: it must be written, or attempts > to Close such blocks should fail. If for whatever reason we skipped > the metadata append, we could end up with block IDs whose blocks > can't be found on startup. Thanks a lot Adar for the summary! -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 16: (4 comments) http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 423: class TestCFileBothCacheTypesCloseTypes : public TestCFile, > You should be able to use this to replace TestCFileBothCacheTypes. Here's w Done Line 449: if (desc.close_type == CLOSE) FLAGS_cfile_close_blocks_on_finish = true; > If the default value of this gflag changes to true, we'll never test the fa Done http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS16, Line 59: // - users could always change this to "close", which slows down throughput : // but may improve write latency. > This part isn't quite right anymore. Should the entire comment be rewritten Done PS16, Line 63: > Nit: got an extra space here. Done -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#17). Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that for LBM container can be reused without flushing in-flight writable blocks to disk. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 591 insertions(+), 309 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/17 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] rpc: move ConnectionId to its own file
Hello Sailesh Mukil, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7685 to look at the new patch set (#2). Change subject: rpc: move ConnectionId to its own file .. rpc: move ConnectionId to its own file This class was previously implemented inside of outbound_call.{cc,h} where it didn't quite belong. In the several years since the code was initially written we've moved more towards a "single class per header" model unless the classes are truly trivial or really tightly coupled. Neither is the case here. Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 --- M src/kudu/rpc/CMakeLists.txt A src/kudu/rpc/connection_id.cc A src/kudu/rpc/connection_id.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/proxy.h M src/kudu/rpc/reactor.h M src/kudu/rpc/rpc_context.cc 8 files changed, 174 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/7685/2 -- To view, visit http://gerrit.cloudera.org:8080/7685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7687 to look at the new patch set (#2). Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies This modifies the constructor of RPC proxies (generated and otherwise) to take the remote hostname in addition to the existing resolved Sockaddr parameter. The hostname is then passed into the ConnectionId object, and plumbed through to the SASL client in place of the IP address that was used previously. The patch changes all of the construction sites of Proxy to fit the new interface. In most of the test cases, we don't have real hostnames, so we just use the dotted-decimal string form of the remote Sockaddr, which matches the existing behavior. In the real call sites, we have actual host names typically specified by the user, and in those cases we'll need to pass those into the proxy. In a few cases, they were conveniently available in the same function that creates the proxy. In others, they are relatively far away, so this patch just uses the dotted-decimal string and leaves TODOs. In the case that Kerberos is not configured, this change should have no effect since the hostname is ignored by SASL "plain". In the case that Kerberos is configured with 'rdns=true', they also have no effect, because the krb5 library will resolve and reverse the hostname from the IP as it did before. In the case that 'rdns=false', this moves us one step closer to fixing KUDU-2032 by getting a hostname into the SASL library. I verified that, if I set 'rdns = false' on a Kerberized client, I'm now able to run 'kudu master status ' successfully where it would not before. This tool uses a direct proxy instantiation where the hostname was easy to plumb in. 'kudu table list ' still does not work because it uses the client, which wasn't convenient to plumb quite yet. Given that this makes incremental improvement towards fixing the issue without any regression, and is already a fairly wide patch, I hope to commit this and then address the remaining plumbing in a separate patch. Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98 --- M src/kudu/client/client-internal.cc M src/kudu/client/master_rpc.cc M src/kudu/client/meta_cache.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/integration-tests/create-table-stress-test.cc M src/kudu/integration-tests/exactly_once_writes-itest.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/internal_mini_cluster.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/rpc/connection.h M src/kudu/rpc/connection_id.cc M src/kudu/rpc/connection_id.h M src/kudu/rpc/exactly_once_rpc-test.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/protoc-gen-krpc.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/proxy.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-bench.cc M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tserver/heartbeater.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server_test_util.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h 42 files changed, 232 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/7687/2 -- To view, visit http://gerrit.cloudera.org:8080/7687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: some small cleanup in ConnectionId
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7686 to look at the new patch set (#2). Change subject: rpc: some small cleanup in ConnectionId .. rpc: some small cleanup in ConnectionId Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361 --- M src/kudu/rpc/connection_id.cc M src/kudu/rpc/connection_id.h M src/kudu/rpc/proxy.cc M src/kudu/rpc/user_credentials.cc M src/kudu/rpc/user_credentials.h 5 files changed, 13 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/7686/2 -- To view, visit http://gerrit.cloudera.org:8080/7686 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] gutil: remove use of deprecated headers
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7684 to look at the new patch set (#2). Change subject: gutil: remove use of deprecated headers .. gutil: remove use of deprecated headers We've been on C++11 for a long time now. This commit removes usage of ext/hash_map and ext/hash_set headers in favor of the now-standardized unordered_map and unordered_set. Additionally, it removes our specializations of __gnucxx::hash<> and instead changes to specializing std::hash<> where appropriate. Some of those specializations are now part of the standard and have been removed as necessary to fix compilation. This does not yet remove the -Wno-deprecated setting in the top-level CMakeLists.txt, since it appears we have a lot of usage of our own internal deprecated functions. Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b --- M CMakeLists.txt M src/kudu/gutil/CMakeLists.txt M src/kudu/gutil/endian.h M src/kudu/gutil/hash/hash.cc M src/kudu/gutil/hash/hash.h M src/kudu/gutil/strings/serialize.cc M src/kudu/gutil/strings/serialize.h M src/kudu/gutil/strings/split.cc M src/kudu/gutil/strings/split.h 9 files changed, 40 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/7684/2 -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: some small cleanup in ConnectionId
Todd Lipcon has posted comments on this change. Change subject: rpc: some small cleanup in ConnectionId .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7686/1/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: PS1, Line 68: set_real_user > nit: maybe, it's worth updating the signature of UserCredentials::set_real_ Done -- To view, visit http://gerrit.cloudera.org:8080/7686 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rpc: move ConnectionId to its own file
Todd Lipcon has posted comments on this change. Change subject: rpc: move ConnectionId to its own file .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7685/1//COMMIT_MSG Commit Message: PS1, Line 10: which didn't > nit: which _it_ didn't ? Done http://gerrit.cloudera.org:8080/#/c/7685/1/src/kudu/rpc/proxy.h File src/kudu/rpc/proxy.h: PS1, Line 26: #include "kudu/rpc/outbound_call.h" > still needed? nope, good catch. -- To view, visit http://gerrit.cloudera.org:8080/7685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] gutil: remove use of deprecated headers
Alexey Serbin has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7684/1/src/kudu/gutil/hash/hash.h File src/kudu/gutil/hash/hash.h: PS1, Line 214: #if defined(__GNUC__) : // Use our nice hash function for strings : template : struct hash > { : size_t operator()(const std::basic_string<_CharT, _Traits, _Alloc>& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : : // they don't define a hash for const string at all : template<> struct hash { : size_t operator()(const std::string& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : #endif // defined(__GNUC__) > I guess this one causes problems when building with libc++ (worked ok with SGTM :) -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] gutil: remove use of deprecated headers
Todd Lipcon has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7684/1/src/kudu/gutil/hash/hash.h File src/kudu/gutil/hash/hash.h: PS1, Line 214: #if defined(__GNUC__) : // Use our nice hash function for strings : template : struct hash > { : size_t operator()(const std::basic_string<_CharT, _Traits, _Alloc>& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : : // they don't define a hash for const string at all : template<> struct hash { : size_t operator()(const std::string& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : #endif // defined(__GNUC__) > Does it make sense to remove this as well? I guess this one causes problems when building with libc++ (worked ok with libstdcxx). Will remove and cross fingers :) -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2098. Drop Spark 1 Support
Grant Henke has uploaded a new change for review. http://gerrit.cloudera.org:8080/7690 Change subject: KUDU-2098. Drop Spark 1 Support .. KUDU-2098. Drop Spark 1 Support Spark 2 has been available for over a year and the value of shipping a seperate Spark 1 artifact is decreasing. In the case where another Spark 1 release occurs or bug fixes are needed, updates to the Kudu Spark 1 integration can still exist via maintenance relases. This allows us to clean up deprecated mehods that are used for compatibility purposes and simplifies the build. Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537 --- M build-support/jenkins/build-and-test.sh M java/README.md M java/gradle/dependencies.gradle M java/kudu-spark-tools/pom.xml M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala M java/kudu-spark/build.gradle M java/kudu-spark/pom.xml M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala R java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala D java/kudu-spark/src/main/spark1/org/apache/kudu/spark/kudu/package.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala M java/pom.xml 18 files changed, 83 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/7690/1 -- To view, visit http://gerrit.cloudera.org:8080/7690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke
[kudu-CR] [java] Update outdated dependencies
Jean-Daniel Cryans has posted comments on this change. Change subject: [java] Update outdated dependencies .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7647 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java] Update outdated dependencies
Adar Dembo has posted comments on this change. Change subject: [java] Update outdated dependencies .. Patch Set 7: Code-Review+2 Maybe JD or Dan want to take a look too? -- To view, visit http://gerrit.cloudera.org:8080/7647 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] separate DataDirManager from BlockManagers
Adar Dembo has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS8, Line 144: : : : : : : : Don't we need to preserve this in some capacity? http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 378: // Goes through the data dirs in 'uuid_indices' and populates > Done. Ah interesting point; how about, "provided to the constructor" Yeah, that's more clear. http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS8, Line 231: qcquired acquired http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 164 Maybe restore this comment? Line 71: return value == "log" || value == "file"; On macOS can you set this up so that "file" is the only valid option? Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_, Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by FsManager::Open, we'll end up creating the DataDirManager twice, once in CreateNew() and once in OpenExisting(). It's not the biggest deal I guess, but maybe it's fixable without too much work? -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Andrew Wong has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 146: bm_.reset(); > Why does the block manager have to be destroyed separately up here? Why isn Done http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 289: LOG(INFO) << "Constructing a new DataDirManager"; > Still need this? Done Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity))); > Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. A Done Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity))); > warning: passing result of std::move() as a const reference argument; no mo Done PS7, Line 339: canonicalized_data_roots.insert(canonicalized_data_roots.end(), : canonicalized_data_roots_set.begin(), : canonicalized_data_roots_set.end()); > Isn't this logically the same as the returned values from the calls to FsM Not quite, it removes duplicates, but in any case, I've taken this out and moved it back to the FsManager. Line 344: dd_manager->reset(new DataDirManager(env, opts, canonicalized_data_roots)); > warning: parameter 'opts' is passed by value and only copied once; consider Done PS7, Line 350: const int kFlagCreateTestHolePunch = 0x1; : const int kFlagCreateFsync = 0x2; > But why bother with flags at all? Flags are a means of defining an API and Ah, makes sense, way clearer. Done. http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS6, Line 229: // Shuts down all directories' thread pools. : void Shutdown(); : : // Waits on all directories' thread pools. : void WaitOnClosures(); : : // Initializes the data directories on disk. : // : // Returns an error if initialized directories already exist. : Status Create(); : : // Opens existing data roots from disk and indexes the files found. : // > Would it be possible to convert the static Init and the Create and/or Open Done. It was mainly a matter of ordering of canonicalization. I.e. the FsManager needs the canonicalized roots before doing anything; I thought I could push some of it to the directory manager, but I think to do a complete job on that might require making the WAL dir known to the directory manager too. Originally I'd wanted to have the directory manager handle the canonicalization failures, but I realize we might be able to get by doing it at the FsManager level (e.g. by prepending an unsanitized prefix to the non-canonicalized path). In any case, I've taken out the canonicalization changes since I don't think they're necessary for this patch, and probably won't be for handling failures at startup. http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS7, Line 200: Defaultas to fales > Defaults to false Done Line 351: DataDirManager(Env* env, > warning: function 'kudu::fs::DataDirManager::DataDirManager' has a definiti Done Line 378: // The canonicalized roots provided at construction time, taken verbatim. > I understand what this means, but "construction time" is a little weird bec Done. Ah interesting point; how about, "provided to the constructor" -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] separate DataDirManager from BlockManagers
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7602 to look at the new patch set (#8). Change subject: separate DataDirManager from BlockManagers .. separate DataDirManager from BlockManagers Currently, the DataDirManager is owned by the BlockManagers. Since only blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under the roots specified by 'fs_data_dirs'), this hierarchy made sense. However, it would be nice to track other files that fall within 'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the directory manager from the block manager makes this more feasible. This introduces a number of changes to the FsManager, BlockManager, and DataDirManager with respect to initialization. - BlockManagers no longer have a Create() function - BlockManager's dtor will now just wait for DataDir closures to finish instead of shutting down the DataDirManager as to not directly affect the DataDirManager - DataDirManagers now have two static constructors: one to open an existing layout, and another to create and open a new layout - FsManagers will only create the BlockManager when opening an fs layout - A validator is added to check the value of FLAGS_block_manager before any of the above initialization can occur Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace --- M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/util/path_util.cc M src/kudu/util/path_util.h 16 files changed, 471 insertions(+), 325 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/8 -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] handle disk failures during tablet copies
Adar Dembo has posted comments on this change. Change subject: handle disk failures during tablet copies .. Patch Set 1: (3 comments) I imagine Mike will do a more thorough review, but overall looks good to me. http://gerrit.cloudera.org:8080/#/c/7654/1//COMMIT_MSG Commit Message: Line 10: receiving data) and the copy session sources (that sending data). "receive" and "send". Or "are receiving" and "are sending". http://gerrit.cloudera.org:8080/#/c/7654/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS1, Line 525: if (group->uuid_indices().size() != valid_uuid_indices.size()) { : return Status::IOError("Directory group contains a failed directory"); : } : group_uuid_indices = &valid_uuid_indices; Unrelated to this patch? http://gerrit.cloudera.org:8080/#/c/7654/1/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: Line 305: Leftover from a change since removed? Or is this stylistic? -- To view, visit http://gerrit.cloudera.org:8080/7654 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic18d93c218ea13f3086f420a4847cb5e29a47bc7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] rpc: some small cleanup in ConnectionId
Alexey Serbin has posted comments on this change. Change subject: rpc: some small cleanup in ConnectionId .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7686/1/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: PS1, Line 68: set_real_user nit: maybe, it's worth updating the signature of UserCredentials::set_real_user() to fit move semantics as well? -- To view, visit http://gerrit.cloudera.org:8080/7686 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] gutil: remove use of deprecated headers
Alexey Serbin has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7684/1/src/kudu/gutil/hash/hash.h File src/kudu/gutil/hash/hash.h: PS1, Line 214: #if defined(__GNUC__) : // Use our nice hash function for strings : template : struct hash > { : size_t operator()(const std::basic_string<_CharT, _Traits, _Alloc>& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : : // they don't define a hash for const string at all : template<> struct hash { : size_t operator()(const std::string& k) const { : return HashTo32(k.data(), static_cast(k.length())); : } : }; : #endif // defined(__GNUC__) Does it make sense to remove this as well? -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] gutil: remove use of deprecated headers
David Ribeiro Alves has posted comments on this change. Change subject: gutil: remove use of deprecated headers .. Patch Set 1: looks good to me, but clang doens't seem to agree :) -- To view, visit http://gerrit.cloudera.org:8080/7684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No