[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@768 PS6, Line 768: // |DATA\METADATA| NONE EXIST | EXIST && < MIN | EXIST && NO LIVE BLOCKS | EXIST && LIVE BLOCKS| I think it may be better to move this explanation to L546. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: OK Not sure why not auto repair this case as well? http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@830 PS6, Line 830: metadata_size Since based on the current logic, metadata_size is 0 when the metada file does not exist, this condition covers not only empty metadata file + zero data size, but also for missing metadata file + zero data size? If so, could you please update the comment above? Also, it would be more clear to use 's_meta.IsNotFound()' for checking missing metadata file. Same for 'Same for data_size==0'. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@840 PS6, Line 840: quick check whether there is no live blocks nit: quickly check whether or not there is any live blocks. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@867 PS6, Line 867: orphaned metadata file nit: orphaned metadata file with no live blocks. -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 06:06:20 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: basic MergeIterator dominance
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12197 ) Change subject: generic_iterators: basic MergeIterator dominance .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@589 PS7, Line 589: // Move all sub-iterators dominated by 'smallest' back into the main list : // before destroying 'smallest'. I think we need to recheck these sub-iterators for dominance instead of moving them back into states_. The algorithm that establishes dominance relations when the MergeIterator is initialized is greedy, and thus which relations are established is a function of the order of iterators passed to the constructor. Thus, it's possible to start off with "sub-optimal" relations (i.e. relations that will be broken earlier in the merge). Consider this example: - Iter A with block [5, 10] - Iter B with block [16, 20] - Iter C with block [11, 15] If initialization yielded dominance relations A>B and A>C, when we fully exhaust A we should establish C>B. The current code would just throw both back into states_. Mike/Todd, what do you guys think? -- To view, visit http://gerrit.cloudera.org:8080/12197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If59d831240af15bfa7ef5709ec3d105d13b28322 Gerrit-Change-Number: 12197 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 02:13:39 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: move iterator declarations into cc file
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: move iterator declarations into cc file
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. Patch Set 3: Verified+1 Overriding Jenkins, unrelated test failure. -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 02:04:07 + Gerrit-HasComments: No
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 01:52:31 + Gerrit-HasComments: No
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hello Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12239 to look at the new patch set (#6). Change subject: [fs]: delete the (orphaned) metadata file when repairing .. [fs]: delete the (orphaned) metadata file when repairing With KUDU-2636 fixed, we now support deleting dead containers at runtime. Because this involves deleting a pair of files, there's a time window in which it's possible for there to be just one file. A well-timed crash or kill -9 can make this a permanent state that fails the log block manager at startup. Indeed, a loop of dense_node-itest failed 2/1000 times in this way. This patch fixes the problem by detecting "orphaned" metadata files at startup and deleting them. A metadata file is orphaned if it has no corresponding data file and if it contains nothing but dead blocks. Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 2 files changed, 346 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/6 -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] revert test changes from KUDU-2236
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12264 ) Change subject: revert test changes from KUDU-2236 .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 Gerrit-Change-Number: 12264 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Jan 2019 01:35:52 + Gerrit-HasComments: No
[kudu-CR] revert test changes from KUDU-2236
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12264 ) Change subject: revert test changes from KUDU-2236 .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12264/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12264/1//COMMIT_MSG@10 PS1, Line 10: a89c8f39544337c6ff36cdd28edec4fadac8427c) > This refers to a gerrit Change-Id; do you have a commit hash for it? Oops, done -- To view, visit http://gerrit.cloudera.org:8080/12264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 Gerrit-Change-Number: 12264 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Jan 2019 01:34:06 + Gerrit-HasComments: Yes
[kudu-CR] revert test changes from KUDU-2236
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12264 ) Change subject: revert test changes from KUDU-2236 .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12264/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12264/1//COMMIT_MSG@10 PS1, Line 10: I7aafdc0eca00e743048ecc099dcb3241ce7ac8ad This refers to a gerrit Change-Id; do you have a commit hash for it? -- To view, visit http://gerrit.cloudera.org:8080/12264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 Gerrit-Change-Number: 12264 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Jan 2019 01:32:33 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2236: use debug logging where appropriate
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/8961 ) Change subject: KUDU-2236: use debug logging where appropriate .. Abandoned Seems to be fixed in https://github.com/apache/kudu/commit/ead756844ce9ada904fcc3666df25692f63e76b8 -- To view, visit http://gerrit.cloudera.org:8080/8961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661 Gerrit-Change-Number: 8961 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] revert test changes from KUDU-2236
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12264 to review the following change. Change subject: revert test changes from KUDU-2236 .. revert test changes from KUDU-2236 A couple of changes (d78b2727d1246069b2006ee652c3ff9a2005601c, I7aafdc0eca00e743048ecc099dcb3241ce7ac8ad) for KUDU-2236 went in to make testCloseShortlyAfterOpen less flaky, but failed to actually fix the underlying logging regression. Commit ead756844ce9ada904fcc3666df25692f63e76b8 fixed the regression, so this patch restores the test to its former coverage. I looped the test and it passed 200/200 runs, compared to the failure rate of 20-50% reported in the Jira. The above commit added a similar test, but given the tests test different things, I've left both in. Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 --- M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/12264/1 -- To view, visit http://gerrit.cloudera.org:8080/12264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 Gerrit-Change-Number: 12264 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo
[kudu-CR] revert test changes from KUDU-2236
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12264 to look at the new patch set (#2). Change subject: revert test changes from KUDU-2236 .. revert test changes from KUDU-2236 A couple of changes (d78b2727d1246069b2006ee652c3ff9a2005601c, a89c8f39544337c6ff36cdd28edec4fadac8427c) for KUDU-2236 went in to make testCloseShortlyAfterOpen less flaky, but failed to actually fix the underlying logging regression. Commit ead756844ce9ada904fcc3666df25692f63e76b8 fixed the regression, so this patch restores the test to its former coverage. I looped the test and it passed 200/200 runs, compared to the failure rate of 20-50% reported in the Jira. The above commit added a similar test, but given the tests test different things, I've left both in. Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 --- M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/12264/2 -- To view, visit http://gerrit.cloudera.org:8080/12264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia93a454b06a40738152cc000d55caa197b64d461 Gerrit-Change-Number: 12264 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] switch all iterators to unique ptr
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/1 to look at the new patch set (#3). Change subject: switch all iterators to unique_ptr .. switch all iterators to unique_ptr We've suffered from a longstanding wart in the iterator subsystem: a mix of raw and various kinds of smart pointers. The usage of shared_ptr was particularly vexing as it suggested that iterators had shared ownership when in fact they didn't. This yak shave of a patch addresses all of those issues by converting all of the iterator pointer types to unique_ptr. I snuck in some clang-tidy suggestions too, but otherwise the cleanup here should narrowly scoped. Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 --- M src/kudu/cfile/cfile-test-base.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/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/generic_iterators.h M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/master/sys_catalog.cc M src/kudu/tablet/all_types-scan-correctness-test.cc M src/kudu/tablet/cfile_set-test.cc M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h M src/kudu/tablet/compaction.cc M src/kudu/tablet/composite-pushdown-test.cc M src/kudu/tablet/delta_compaction-test.cc M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltafile-test.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diff_scan-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/major_delta_compaction-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/mt-rowset_delta_compaction-test.cc M src/kudu/tablet/mt-tablet-test.cc M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet-decoder-eval-test.cc M src/kudu/tablet/tablet-pushdown-test.cc M src/kudu/tablet/tablet-schema-test.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 57 files changed, 294 insertions(+), 302 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/1/3 -- To view, visit http://gerrit.cloudera.org:8080/1 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Gerrit-Change-Number: 1 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java] deflake tests that use KuduTestHarness.findLeaderMasterServer
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12263 ) Change subject: [java] deflake tests that use KuduTestHarness.findLeaderMasterServer .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5612619d1b9e30df7d627f2370d60ce2aa812713 Gerrit-Change-Number: 12263 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Jan 2019 01:27:41 + Gerrit-HasComments: No
[kudu-CR] generic iterators: assorted cleanup
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12156 ) Change subject: generic_iterators: assorted cleanup .. generic_iterators: assorted cleanup This patch cleans up the MergeIterator and makes it look more like the UnionIterator: 1. Addressed a long-standing TODO about schema verification. 2. Extracted the schema from the sub-iterators. Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544 Reviewed-on: http://gerrit.cloudera.org:8080/12156 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/generic_iterators.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/tablet.cc 5 files changed, 100 insertions(+), 74 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544 Gerrit-Change-Number: 12156 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Reviewed-on: http://gerrit.cloudera.org:8080/12157 Tested-by: Adar Dembo Reviewed-by: Mike Percy --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/rowblock.h 3 files changed, 97 insertions(+), 36 deletions(-) Approvals: Adar Dembo: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: prep for MergeIterator dominance
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12196 ) Change subject: generic_iterators: prep for MergeIterator dominance .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a8db5973dc09b7271e05b3cc28025b7a2a9e21b Gerrit-Change-Number: 12196 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 01:05:10 + Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 10: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 24 Jan 2019 00:58:06 + Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc@463 PS8, Line 463: // Enforce access control, and make elections much more frequent. > These all have an effect _after_ the cluster has been started? Good callout, a couple of them do affect state set at table create time (e.g. the periodic timers). -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 24 Jan 2019 00:56:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12122 to look at the new patch set (#10). Change subject: KUDU-2543 pt 2: pass around default authz tokens .. KUDU-2543 pt 2: pass around default authz tokens Adds authz token generation to the master's GetTableSchema endpoint, with which clients can authorize themselves for specific tables. A client will cache these tokens and use them appropriately for RPCs that need them (e.g. Writes and Scans), reacquiring them when receiving word that they are expired. This is tested in the following ways: - unit tests for the new client-side cache for authz tokens - parameterized the token expiration test for authn and authz tokens to have varying token expirations, testing when authn tokens expire but not authz tokens, and vice versa - added various tests to ensure the client behaves correctly in various non-happy cases Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 --- M src/kudu/client/CMakeLists.txt A src/kudu/client/authz_token_cache.cc A src/kudu/client/authz_token_cache.h M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/auth_token_expire-itest.cc A src/kudu/integration-tests/authz_token-itest.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/rpc/retriable_rpc.h M src/kudu/rpc/rpc.h M src/kudu/util/test_util.h 20 files changed, 1,335 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/10 -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [java] deflake tests that use KuduTestHarness.findLeaderMasterServer
Hello Alexey Serbin, Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12263 to review the following change. Change subject: [java] deflake tests that use KuduTestHarness.findLeaderMasterServer .. [java] deflake tests that use KuduTestHarness.findLeaderMasterServer >From time to time I'd see test failures like these: 10:10:16.018 [INFO - Test worker] (KuduTestHarness.java:147) Creating a new Kudu client... ... 10:10:16.036 [WARN - New I/O worker #158] (ConnectToCluster.java:278) None of the provided masters 127.6.239.254:42291,127.6.239.252:41769,127.6.239.253:41053 is a leader; will retry ... 10:10:16.060 [ERROR - Test worker] (RetryRule.java:80) testExportAuthenticationCredentialsDuringLeaderElection(org.apache.kudu.client.TestKuduClient): failed attempt 1 org.apache.kudu.client.NoLeaderFoundException: Master config (127.6.239.254:42291,127.6.239.252:41769,127.6.239.253:41053) has no leader. at org.apache.kudu.client.ConnectToCluster.incrementCountAndCheckExhausted(ConnectToCluster.java:279) at org.apache.kudu.client.ConnectToCluster.access$100(ConnectToCluster.java:47) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:323) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:312) at com.stumbleupon.async.Deferred.doCall(Deferred.java:1280) at com.stumbleupon.async.Deferred.runCallbacks(Deferred.java:1259) at com.stumbleupon.async.Deferred.callback(Deferred.java:1002) at org.apache.kudu.client.KuduRpc.handleCallback(KuduRpc.java:247) at org.apache.kudu.client.KuduRpc.callback(KuduRpc.java:294) at org.apache.kudu.client.RpcProxy.responseReceived(RpcProxy.java:269) at org.apache.kudu.client.RpcProxy.access$000(RpcProxy.java:59) at org.apache.kudu.client.RpcProxy$1.call(RpcProxy.java:131) at org.apache.kudu.client.RpcProxy$1.call(RpcProxy.java:127) at org.apache.kudu.client.Connection.messageReceived(Connection.java:391) at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at org.apache.kudu.client.Connection.handleUpstream(Connection.java:243) I'd look through the code and wonder how this could happen if KUDU-2387 was indeed fixed. Today I finally noticed that KuduTestHarness.findLeaderMasterServer calls getMasterTableLocationsPB directly, and I remembered that without applying the same logic as in KUDU-2387, such calls will not retry. After adding a catch block to findLeaderMasterServer and transforming the thrown exception, I got a useful stack trace confirming the problem: 10:51:53.627 [ERROR - Test worker] (RetryRule.java:80) testExportAuthenticationCredentialsDuringLeaderElection(org.apache.kudu.client.TestKuduClient): failed attempt 1 org.apache.kudu.client.NoLeaderFoundException: Master config (127.11.27.62:40985,127.11.27.60:37593,127.11.27.61:37931) has no leader. at org.apache.kudu.client.KuduException.transformException(KuduException.java:110) at org.apache.kudu.test.KuduTestHarness.findLeaderMasterServer(KuduTestHarness.java:281) at org.apache.kudu.test.KuduTestHarness.restartLeaderMaster(KuduTestHarness.java:329) at org.apache.kudu.client.TestKuduClient.runTestCallDuringLeaderElection(TestKuduClient.java:1124) at org.apache.kudu.client.TestKuduClient.testExportAuthenticationCredentialsDuringLeaderElection(TestKuduClient.java:1150) ... Suppressed: org.apache.kudu.client.KuduException$OriginalException: Original asynchronous stack trace at org.apache.kudu.client.ConnectToCluster.incrementCountAndCheckExhausted(ConnectToCluster.java:279) at org.apache.kudu.client.ConnectToCluster.access$100(ConnectToCluster.java:47) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:323) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:312) at com.stumbleupon.async.Deferred.doCall(Deferred.java:1280) at com.stumbleupon.async.Deferred.runCallbacks(Deferred.java:1259) at com.stumbleupon.async.Deferred.callback(Deferred.java:1002) at org.apache.kudu.client.KuduRpc.handleCallback(KuduRpc.java:247) <...netty> This patch fixes these issues by providing an alternate way to find the leader master: if not known, make some call that will only succeed if the leader master is known, then try again. Without the fix, 29/1000 runs of TestKuduClient failed with this error, either in testExportAuthenticationCredentialsDuringLeaderElection or in testGetHiveMetastoreConfigDuringLeaderElection. With the fix, 0/1000 runs of TestKuduClient failed. Change-Id: I5612619d1b9e30df7d627f2370d60ce2aa812713 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M
[kudu-CR] generic iterators: move iterator declarations into cc file
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 00:41:08 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12173/6/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/6/docker/bootstrap-env.sh@165 PS6, Line 165: "Unsupported OS" > What about SLES? We don't want to support it at all or it's just coming la There are no official SLES images in docker hub. There is OpenSUSE that we could build on, but I didn't want to put in the effort right now. We can always add it later. -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Jan 2019 00:41:29 + Gerrit-HasComments: Yes
[kudu-CR] De-flake sentry tests
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11961 ) Change subject: De-flake sentry tests .. De-flake sentry tests Currently, sentry_client-test and sentry_authz_provider-test are still flaky due to possible port collision. To avoid such race condition, this patch moves the util function that finds the bind address for a daemon based on binding mode to test_util, and updates sentry-test-base to use it to pick up a unique bind address. Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Reviewed-on: http://gerrit.cloudera.org:8080/11961 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 13 files changed, 168 insertions(+), 142 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Gerrit-Change-Number: 11961 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287 PS6, Line 287: after being told go retrieve a new token > It's verified by the fact that we got a new token at L292, and there are VL Sounds good. -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 23:28:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 ) Change subject: KUDU-2543 pt 1: basic checks for authz tokens .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 23:29:03 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12173/6/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/6/docker/bootstrap-env.sh@165 PS6, Line 165: "Unsupported OS" What about SLES? We don't want to support it at all or it's just coming later? -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 23:20:59 + Gerrit-HasComments: Yes
[kudu-CR] switch all iterators to unique ptr
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/1 ) Change subject: switch all iterators to unique_ptr .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/1/2/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/1/2/src/kudu/tablet/cfile_set.h@a84 PS2, Line 84: > no longer virtual? Not sure why it ever was; no other class inherits from CFileSet. GetBounds should likewise be de-virtualized. -- To view, visit http://gerrit.cloudera.org:8080/1 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Gerrit-Change-Number: 1 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 23:19:52 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add an initial docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@58 PS3, Line 58: \ > That meant the ending backslash is not needed for the very last line of the Ah cool, my latest patches took care of that. -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 23:15:53 + Gerrit-HasComments: Yes
[kudu-CR] switch all iterators to unique ptr
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/1 ) Change subject: switch all iterators to unique_ptr .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/1/2/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/1/2/src/kudu/tablet/cfile_set.h@a84 PS2, Line 84: no longer virtual? -- To view, visit http://gerrit.cloudera.org:8080/1 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Gerrit-Change-Number: 1 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 23:15:08 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. I looped master_sentry-itest 1000 times (which used UNIQUE_LOOPBACK mode) and all passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1548189107.129474 [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Reviewed-on: http://gerrit.cloudera.org:8080/11956 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 532 insertions(+), 77 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc@217 PS6, Line 217: // Some entries are randomly deselected in order to exercise the selection > Think of the deselection as yet another predicate: besides omitting certain Thanks. -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 23:04:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 9: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/8/src/kudu/integration-tests/authz_token-itest.cc@463 PS8, Line 463: // Enforce access control, and make elections much more frequent. These all have an effect _after_ the cluster has been started? -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:40:51 + Gerrit-HasComments: Yes
[kudu-CR] [spark] Add some logging to trace KuduContext operations
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12252 ) Change subject: [spark] Add some logging to trace KuduContext operations .. [spark] Add some logging to trace KuduContext operations This patch adds some logging to help track how long an entire KuduContext operation takes and also how long each part takes on each executor. This information has been sorely lacking in some cases where Spark's laziness makes attributing slowness to Kudu (vs other components of the Spark job) very difficult. Unfortunately, it's not as straightforward to add this sort of logging to reading from Kudu (KuduRDD) because Spark may lazily read batches from Kudu, and batches may be small enough that logging for each batch is so verbose that it is not useful. I tested this patch manually on a 3-node cluster and confirmed I saw the expected log messages on the driver and on the executors, e.g. 19/01/22 15:18:13 INFO kudu.KuduContext: applying operations of type 'insert' to table 'impala::default.aaa' 19/01/22 15:18:13 INFO kudu.KuduContext: applied 1 operations of type 'insert' to table 'impala::default.aaa' Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Reviewed-on: http://gerrit.cloudera.org:8080/12252 Reviewed-by: Grant Henke Tested-by: Will Berkeley --- M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala 2 files changed, 26 insertions(+), 3 deletions(-) Approvals: Grant Henke: Looks good to me, approved Will Berkeley: Verified -- To view, visit http://gerrit.cloudera.org:8080/12252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Gerrit-Change-Number: 12252 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docker] Add an initial docker build
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh@25 PS5, Line 25: # https://www.cloudera.com/documentation/enterprise/latest/topics/kudu_troubleshooting.html#slow_dns_nscd Let's link to upstream docs instead: https://kudu.apache.org/docs/troubleshooting.html#slow_dns_nscd http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh@85 PS5, Line 85: then Nit: add a space before -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:45:27 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: assorted cleanup
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 ) Change subject: generic_iterators: assorted cleanup .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544 Gerrit-Change-Number: 12156 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 22:52:24 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:51:49 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12173 to look at the new patch set (#6). Change subject: [docker] Add an initial docker build .. [docker] Add an initial docker build This patch adds an initial Docker file which defines a multistage build that supports creating various Docker images for Apache Kudu development. This work is experimental and the choices and structure may change wih follow on commits. Some things these images could be used for include: - demos/examples - pre-built binaries including thirdparty - multi-os testing - multi-os tools like build_mini_cluster_binaries.sh - external integration testing Some of the open tasks for the future are: - Handle all build types. - Optimize image sizes. - Add kudu user to runtime images. - Add an upload script with good tagging. - Add healthchecks. Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 --- A .dockerignore A docker/Dockerfile A docker/Dockerfile-files A docker/README.adoc A docker/bootstrap-env.sh A docker/docker-build.sh A docker/kudu-entrypoint.sh M thirdparty/build-definitions.sh 8 files changed, 817 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/12173/6 -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [docker] Add an initial docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh@25 PS5, Line 25: # https://www.cloudera.com/documentation/enterprise/latest/topics/kudu_troubleshooting.html#slow_dns_nscd > Let's link to upstream docs instead: https://kudu.apache.org/docs/troublesh Done http://gerrit.cloudera.org:8080/#/c/12173/5/docker/bootstrap-env.sh@85 PS5, Line 85: then > Nit: add a space before Done -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:48:42 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 12: Verified+1 Code-Review+2 Overriding Jenkins, the failure is a known flake and unrelated to this patch. -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:39:32 + Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 ) Change subject: KUDU-2543 pt 1: basic checks for authz tokens .. Patch Set 12: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:41:03 + Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 ) Change subject: KUDU-2543 pt 1: basic checks for authz tokens .. Patch Set 12: Verified+1 Failure was a known flake caused by KUDU-1736 -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:41:01 + Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12122 to look at the new patch set (#9). Change subject: KUDU-2543 pt 2: pass around default authz tokens .. KUDU-2543 pt 2: pass around default authz tokens Adds authz token generation to the master's GetTableSchema endpoint, with which clients can authorize themselves for specific tables. A client will cache these tokens and use them appropriately for RPCs that need them (e.g. Writes and Scans), reacquiring them when receiving word that they are expired. This is tested in the following ways: - unit tests for the new client-side cache for authz tokens - parameterized the token expiration test for authn and authz tokens to have varying token expirations, testing when authn tokens expire but not authz tokens, and vice versa - added various tests to ensure the client behaves correctly in various non-happy cases Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 --- M src/kudu/client/CMakeLists.txt A src/kudu/client/authz_token_cache.cc A src/kudu/client/authz_token_cache.h M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/auth_token_expire-itest.cc A src/kudu/integration-tests/authz_token-itest.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/rpc/retriable_rpc.h M src/kudu/rpc/rpc.h M src/kudu/util/test_util.h 20 files changed, 1,332 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/9 -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Andrew Wong has removed a vote on this change. Change subject: KUDU-2543 pt 1: basic checks for authz tokens .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [java] log warning if applying operation on a closed session
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12237 ) Change subject: [java] log warning if applying operation on a closed session .. [java] log warning if applying operation on a closed session Closed sessions won't flush on another close() call, so it's somewhat dubious to apply() an operation to a closed session. This change adds a warning if apply() is used with a closed session. Originally this patch enforced the new behavior via precondition, but that's a breaking change for clients that relied on the old broken behavior. Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Reviewed-on: http://gerrit.cloudera.org:8080/12237 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 2 files changed, 30 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [spark] Add metrics to kudu-spark
Will Berkeley has removed a vote on this change. Change subject: [spark] Add metrics to kudu-spark .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [spark] Add some logging to trace KuduContext operations
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12252 ) Change subject: [spark] Add some logging to trace KuduContext operations .. Patch Set 2: Verified+1 I didn't rebase to pick up the fix for the Python pandas issue. -- To view, visit http://gerrit.cloudera.org:8080/12252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Gerrit-Change-Number: 12252 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 21:39:31 + Gerrit-HasComments: No
[kudu-CR] [spark] Add metrics to kudu-spark
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12251 ) Change subject: [spark] Add metrics to kudu-spark .. Patch Set 2: Verified+1 I didn't rebase to pick up the Python pandas issue fix. -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 21:39:59 + Gerrit-HasComments: No
[kudu-CR] [spark] Add metrics to kudu-spark
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12251 ) Change subject: [spark] Add metrics to kudu-spark .. [spark] Add metrics to kudu-spark This adds some basic accumulator metrics to kudu-spark. These accumulators appear for each stage involving a KuduContext, with values broken down by executor. Follow-ups will add some more sophisticated metrics and use metrics to add additional logging. In addition to the unit tests, I tested this on a 3-node cluster and confirmed I was able to see the accumulator values for stages and individual tasks on the Spark application's web UI, and that were correct. Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Reviewed-on: http://gerrit.cloudera.org:8080/12251 Reviewed-by: Grant Henke Tested-by: Will Berkeley --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.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/KuduRDDTest.scala 6 files changed, 74 insertions(+), 23 deletions(-) Approvals: Grant Henke: Looks good to me, approved Will Berkeley: Verified -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [spark] Add write duration histograms
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12261 ) Change subject: [spark] Add write duration histograms .. Patch Set 1: Verified+1 Python pandas issue fix not rebased on, and an unrelated C++ flake. -- To view, visit http://gerrit.cloudera.org:8080/12261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fd4d380b08bd7d7d5c1e65b79cffb44a9b9d433 Gerrit-Change-Number: 12261 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 21:40:37 + Gerrit-HasComments: No
[kudu-CR] [spark] Add write duration histograms
Will Berkeley has removed a vote on this change. Change subject: [spark] Add write duration histograms .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I0fd4d380b08bd7d7d5c1e65b79cffb44a9b9d433 Gerrit-Change-Number: 12261 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [spark] Add some logging to trace KuduContext operations
Will Berkeley has removed a vote on this change. Change subject: [spark] Add some logging to trace KuduContext operations .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Gerrit-Change-Number: 12252 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docker] Add an initial docker build
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12173 to look at the new patch set (#5). Change subject: [docker] Add an initial docker build .. [docker] Add an initial docker build This patch adds an initial Docker file which defines a multistage build that supports creating various Docker images for Apache Kudu development. This work is experimental and the choices and structure may change wih follow on commits. Some things these images could be used for include: - demos/examples - pre-built binaries including thirdparty - multi-os testing - multi-os tools like build_mini_cluster_binaries.sh - external integration testing Some of the open tasks for the future are: - Handle all build types. - Optimize image sizes. - Add kudu user to runtime images. - Add an upload script with good tagging. - Add healthchecks. Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 --- A .dockerignore A docker/Dockerfile A docker/Dockerfile-files A docker/README.adoc A docker/bootstrap-env.sh A docker/docker-build.sh A docker/kudu-entrypoint.sh M thirdparty/build-definitions.sh 8 files changed, 817 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/12173/5 -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] log warning if applying operation on a closed session
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12237 ) Change subject: [java] log warning if applying operation on a closed session .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 20:23:08 + Gerrit-HasComments: No
[kudu-CR] [spark] Add metrics to kudu-spark
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12251 ) Change subject: [spark] Add metrics to kudu-spark .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 20:18:36 + Gerrit-HasComments: No
[kudu-CR] [spark] Add some logging to trace KuduContext operations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12252 ) Change subject: [spark] Add some logging to trace KuduContext operations .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Gerrit-Change-Number: 12252 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 20:18:49 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12173 to look at the new patch set (#4). Change subject: [docker] Add an initial docker build .. [docker] Add an initial docker build This patch adds an initial Docker file which defines a multistage build that supports creating various Docker images for Apache Kudu development. This work is experimental and the choices and structure may change wih follow on commits. Some things these images could be used for include: - demos/examples - pre-built binaries including thirdparty - multi-os testing - multi-os tools like build_mini_cluster_binaries.sh - external integration testing Some of the open tasks for the future are: - Handle all build types. - Optimize image sizes. - Add kudu user to runtime images. - Add an upload script with good tagging. - Add healthchecks. Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 --- A .dockerignore A docker/Dockerfile A docker/Dockerfile-files A docker/README.adoc A docker/bootstrap-env.sh A docker/docker-build.sh A docker/kudu-entrypoint.sh M thirdparty/build-definitions.sh 8 files changed, 817 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/12173/4 -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [docker] Add an initial docker build
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12173 )
Change subject: [docker] Add an initial docker build
..
Patch Set 3:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/12173/2/docker/Dockerfile
File docker/Dockerfile:
http://gerrit.cloudera.org:8080/#/c/12173/2/docker/Dockerfile@71
PS2, Line 71: # TODO: Remove build dir too?
> Right, that's the issue I ran into and I didn't try to fix it yet. Being ab
I removed all but the llvm directory.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile
File docker/Dockerfile:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@32
PS3, Line 32: ARG BASE_OS=ubuntu:xenial
> I'm curious, why do we default to ubuntu:xenial? In patch set one we used c
ubuntu seems to be a more common choice among ecosystem images (impala for
example) so I set it as the default now that it is supported. My next patch
update adds more support.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@57
PS3, Line 57: #
: # Thirdparty
: # Builds an image that has Kudu's thirdparty dependencies built.
: # This is done in it's own stage so that docker can cache it and
only
: # run it when thirdparty has changes.
: #
> While not necessary, maybe we should consider installing ccache/ninja (thou
I had considered this but wanted to keep things minimal.
ccache and ninja are likely most useful on the kudu-build image which I imagine
would be used as psuedo dev/experimentation containers. Otherwise the standard
build tools/process will be used and the cache will be fresh each build unless
we setup volumes.
Lets address this in a follow up change.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc
File docker/README.adoc:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc@72
PS3, Line 72:
: === kudu-master
: A runtime image with kudu-master as the ENTRYPOINT.
: Uses the kudu-runtime image as a base.
:
: === kudu-tserver
: A runtime image with kudu-tserver as the ENTRYPOINT.
: Uses the kudu-runtime image as a base.
> You might not have run into this if your docker version is new, but it migh
I think I workaround this by setting `--use_hybrid_clock=false` so that `ntp`
isn't needed. That is something that will be addressed when those TODO's are
done.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh
File docker/bootstrap-env.sh:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@28
PS3, Line 28: # Install the prerequisite libraries, if they are not installed.
> I know it's not in the upstream installation docs, but it's probably worth
I added this as a todo. This patch is large and will have many improvements
going forward.
I have found and included a few things that are not in the upstream docs and
planned to add them at some point. Alternatively we could point people to this
script to setup their environment, though I am not sure we want to mix it's
purpose.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@58
PS3, Line 58: \
> drop
Why drop? Not sure what this means?
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@36
PS3, Line 36: autoconf \
: automake \
: cyrus-sasl-devel \
: cyrus-sasl-gssapi \
: cyrus-sasl-plain \
: flex \
: gcc \
: gcc-c++ \
: gdb \
: git \
: java-1.8.0-openjdk-devel \
: krb5-server \
: krb5-workstation \
: libtool \
: make \
: openssl-devel \
: patch \
: pkgconfig \
: redhat-lsb-core \
: rsync \
: unzip \
: vim-common \
: whic
> nit here and below: add an extra indent for the continuation of the command
Done
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh
File docker/docker-build.sh:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@37
PS3, Line 37: --build-arg MAINTAINER="Apache Kudu"
> nit: I think there should be a space between the name and the email
Done
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@45
PS3, Line 45: docker build "${BUILD_ARGS[@]}" -f $ROOT/docker/Dockerfile
--target kudu-base -t kudu-base $ROOT
> I believe the tag should be kudu:* instead of kudu-* and each image should
One of my open follow up items listed in the commit message is "Add an upload
script with good tagging."
I have a lot of options/thoughts on tagging, but given others might too I
wanted to handle that in a separate patch.
I think tagging and uploading thirdparty is useful because you can pull a
pre-built
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: hms_.reset(new hms::MiniHms()); > You're right; not sure how I thought this would help macOS. Done http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc@144 PS11, Line 144: // Check that the port number only changed if the original port was 0 > Nit: this is a bit unclear. Reword as "Check that the port number only chan Done -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:59:16 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 6: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287 PS6, Line 287: after being told go retrieve a new token Is there a way to verify this? e.g. a log? -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:56:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 ) Change subject: KUDU-2543 pt 2: pass around default authz tokens .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/6/src/kudu/integration-tests/authz_token-itest.cc@287 PS6, Line 287: after being told go retrieve a new token > Is there a way to verify this? e.g. a log? It's verified by the fact that we got a new token at L292, and there are VLOG messages in the token cache when new tokens are retrieved. -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 20:01:49 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12122 to look at the new patch set (#7). Change subject: KUDU-2543 pt 2: pass around default authz tokens .. KUDU-2543 pt 2: pass around default authz tokens Adds authz token generation to the master's GetTableSchema endpoint, with which clients can authorize themselves for specific tables. A client will cache these tokens and use them appropriately for RPCs that need them (e.g. Writes and Scans), reacquiring them when receiving word that they are expired. This is tested in the following ways: - unit tests for the new client-side cache for authz tokens - parameterized the token expiration test for authn and authz tokens to have varying token expirations, testing when authn tokens expire but not authz tokens, and vice versa - added various tests to ensure the client behaves correctly in various non-happy cases Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 --- M src/kudu/client/CMakeLists.txt A src/kudu/client/authz_token_cache.cc A src/kudu/client/authz_token_cache.h M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/auth_token_expire-itest.cc A src/kudu/integration-tests/authz_token-itest.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/rpc/retriable_rpc.h M src/kudu/rpc/rpc.h M src/kudu/util/test_util.h 20 files changed, 1,332 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12122/7 -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#12). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. I looped master_sentry-itest 1000 times (which used UNIQUE_LOOPBACK mode) and all passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1548189107.129474 [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 532 insertions(+), 77 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/12 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [spark] Add metrics to kudu-spark
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12251 to look at the new patch set (#2). Change subject: [spark] Add metrics to kudu-spark .. [spark] Add metrics to kudu-spark This adds some basic accumulator metrics to kudu-spark. These accumulators appear for each stage involving a KuduContext, with values broken down by executor. Follow-ups will add some more sophisticated metrics and use metrics to add additional logging. In addition to the unit tests, I tested this on a 3-node cluster and confirmed I was able to see the accumulator values for stages and individual tasks on the Spark application's web UI, and that were correct. Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.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/KuduRDDTest.scala 6 files changed, 74 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/12251/2 -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [spark] Add write duration histograms
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12261 Change subject: [spark] Add write duration histograms .. [spark] Add write duration histograms This adds an additional accumulator metrics to KuduContext writes: write duration histograms. These histograms will show up on the webui and in the driver logs, so it's easier to track how much time is spent writing to Kudu in Spark stages and tasks. Log messages on the driver look like: 19/01/23 11:13:34 INFO kudu.KuduContext: completed insert ops: duration histogram: 25.0%: 14ms, 25.0%: 14ms, 75.0%: 17ms, 75.0%: 17ms, 75.0%: 17ms, 100.0%: 66ms, 100.0%: 66ms The funny repeated values are an artifact of having a cluster with only 3 executors executing 4 tasks. Log messages on executors look like 19/01/23 11:13:34 INFO kudu.KuduContext: applied 69 inserts to table 'impala::default.aaa' in 14ms HdrHistograms need to be shipped between executors and the driver, so their (serialized) size is relevant. Spark users differ in how they serialize, so I didn't put much effort into estimating the serialized size, but based on the conservative formula in [1] the in-memory size of a histogram with 3 significant value digits and storing longs is 4MiB or so. That only happens if the histogram is storing values from 1 to the max trackable long value, which is Long.MAX / 2. More realistically, the values in the duration histogram should be at most 86400 * 1000, the number of milliseconds in a day, and usually much, much smaller. For that range of values, the max footprint is 1MiB. That should be a safe amount of data to ship about semi-frequently along with all the Kudu data (and I'm not counting potential compression as part of serialization). [1]: https://github.com/HdrHistogram/HdrHistogram#footprint-estimation Change-Id: I0fd4d380b08bd7d7d5c1e65b79cffb44a9b9d433 --- M java/gradle/dependencies.gradle M java/kudu-spark/build.gradle A java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala 4 files changed, 114 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/12261/1 -- To view, visit http://gerrit.cloudera.org:8080/12261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0fd4d380b08bd7d7d5c1e65b79cffb44a9b9d433 Gerrit-Change-Number: 12261 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] [spark] Add some logging to trace KuduContext operations
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12252 ) Change subject: [spark] Add some logging to trace KuduContext operations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12252/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: http://gerrit.cloudera.org:8080/#/c/12252/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@246 PS1, Line 246: log.info > Maybe, move this into writeRows() itself? It seems there are enough parame There's two reasons I made it this way: 1. Nicer logs. Insert/upsert/update/delete conjugate differently and use different prepositions, e.g. "inserted" (add -ed) vs. "deleted" (add -d), and "insert into" vs. "delete from". 2. This doesn't feel worse to me than two big `match` statements in `writeRows` to do the logging cleanly. If it bugs you that much, feel free to make a follow-up that changes it :) -- To view, visit http://gerrit.cloudera.org:8080/12252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6741f2584c1bc6b229d10d37297515474318f94c Gerrit-Change-Number: 12252 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 19:57:52 + Gerrit-HasComments: Yes
[kudu-CR] [spark] Add metrics to kudu-spark
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12251 ) Change subject: [spark] Add metrics to kudu-spark .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12251/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: http://gerrit.cloudera.org:8080/#/c/12251/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@69 PS1, Line 69: KA > A Done http://gerrit.cloudera.org:8080/#/c/12251/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala: http://gerrit.cloudera.org:8080/#/c/12251/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala@130 PS1, Line 130: val rowsRead: LongAccumulator > doc? Done http://gerrit.cloudera.org:8080/#/c/12251/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala@156 PS1, Line 156: rowsRead.add(currentIterator.getNumRows) > This is a bit tricky. The rows have been read from Kudu but not necessarily The reason it is done here is because this is about performance measurement (how many rows did executor 1 read v. executor 2, and in how long?), as opposed to checking correctness (I don't see all 1000 rows that I expect...how many did each executor read?). I think for the former it is better to measure what is read from Kudu regardless of whether the rows are consumed by the Spark application. -- To view, visit http://gerrit.cloudera.org:8080/12251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied81c890b3d4510f767d8f023963ff878f398140 Gerrit-Change-Number: 12251 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 23 Jan 2019 19:57:47 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add an initial docker build
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc File docker/README.adoc: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc@72 PS3, Line 72: : === kudu-master : A runtime image with kudu-master as the ENTRYPOINT. : Uses the kudu-runtime image as a base. : : === kudu-tserver : A runtime image with kudu-tserver as the ENTRYPOINT. : Uses the kudu-runtime image as a base. You might not have run into this if your docker version is new, but it might be worth making note of KUDU-2000 in case users try to deploy using older versions of docker (or maybe just note the version of docker you tested with). -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:56:56 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add an initial docker build
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile File docker/Dockerfile: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@57 PS3, Line 57: # : # Thirdparty : # Builds an image that has Kudu's thirdparty dependencies built. : # This is done in it's own stage so that docker can cache it and only : # run it when thirdparty has changes. : # While not necessary, maybe we should consider installing ccache/ninja (though maybe as a follow-on patch if it turns out being a headache). This could be super useful as a developer tool wanting to develop/test on different environments, so it might be helpful to include iterative dev tools out of the box. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@28 PS3, Line 28: # Install the prerequisite libraries, if they are not installed. I know it's not in the upstream installation docs, but it's probably worth adding nscd, or a TODO to add it in case people use this and run into issues: https://www.cloudera.com/documentation/enterprise/latest/topics/kudu_troubleshooting.html#slow_dns_nscd -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:51:48 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 1: basic checks for authz tokens
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 ) Change subject: KUDU-2543 pt 1: basic checks for authz tokens .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:39:55 + Gerrit-HasComments: No
[kudu-CR] [java] log warning if applying operation on a closed session
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12237 ) Change subject: [java] log warning if applying operation on a closed session .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:58:10 + Gerrit-HasComments: No
[kudu-CR] [java] log warning if applying operation on a closed session
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12237 ) Change subject: [java] log warning if applying operation on a closed session .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12237/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12237/4//COMMIT_MSG@14 PS4, Line 14: a breaking change for clients that relied on the old broken behavior. : > Could throwing an exception in the specified case be optional? I guess by I ended up going with Hao's suggestion: soften this into a warning instead. -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:15:05 + Gerrit-HasComments: Yes
[kudu-CR] [java] log warning if applying operation on a closed session
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12237 ) Change subject: [java] log warning if applying operation on a closed session .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12237/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12237/4//COMMIT_MSG@14 PS4, Line 14: a breaking change for clients that relied on the old broken behavior. : > I ended up going with Hao's suggestion: soften this into a warning instead. SGTM -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:23:41 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add an initial docker build
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@30 PS3, Line 30: if [[ -n $(which yum) ]]; > why not just Whoops, ignore this one. -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:20:01 + Gerrit-HasComments: Yes
[kudu-CR] [docker] Add an initial docker build
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: (11 comments) Just skimmed through. It's great that we are about to have that dockerized stuff soon! http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@30 PS3, Line 30: if [[ -n $(which yum) ]]; why not just if $(which yum); then ... fi ? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@58 PS3, Line 58: \ drop http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@36 PS3, Line 36: autoconf \ : automake \ : cyrus-sasl-devel \ : cyrus-sasl-gssapi \ : cyrus-sasl-plain \ : flex \ : gcc \ : gcc-c++ \ : gdb \ : git \ : java-1.8.0-openjdk-devel \ : krb5-server \ : krb5-workstation \ : libtool \ : make \ : openssl-devel \ : patch \ : pkgconfig \ : redhat-lsb-core \ : rsync \ : unzip \ : vim-common \ : whic nit here and below: add an extra indent for the continuation of the command line http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh File docker/kudu-entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@22 PS3, Line 22: KUDU_FLAGS Do those come from the environment? Or that's about modifying this particular script and populating KUDU_FLAGS with something? If the latter, consider introducing the KUDU_FLAGS var with default value "" (i.e. empty). http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@27 PS3, Line 27: resolved ... reached by their DNS name ... This also verifies that network interfaces of master hosts are up and running. BTW, is it a requirement to have master hosts up and running? If not, why to use ping instead of DNS-related utils? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@51 PS3, Line 51: /var/lib/kudu/master Turn into a variable to re-use elsewhere? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@52 PS3, Line 52: $KUDU_FLAGS KUDU_FLAGS is common for both kudu-master and kudu-tservers. Does it make sense to separate those? Also, if KUDU_FLAGS are here for customization, maybe make them last so it would be possible to override some options specified by this script? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@55 PS3, Line 55: /opt/kudu/www Is it guaranteed to exist? Or that doesn't matter? Also, consider turning it into a variable to re-use elsewhere in the script. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@62 PS3, Line 62: /var/lib/kudu/tserver Turn into a variable to re-use elsewhere? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@66 PS3, Line 66: /opt/kudu/www Is it guaranteed to exist? Or that doesn't matter? http://gerrit.cloudera.org:8080/#/c/12173/3/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/thirdparty/build-thirdparty.sh@252 PS3, Line 252: rm -rf $PREFIX/opt/hadoop nit: I'm not sure removing data automatically is a good thing to do. Maybe, just move the old dir X into X.$(date)? -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:16:25 + Gerrit-HasComments: Yes
[kudu-CR] [java] log warning if applying operation on a closed session
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12237 to look at the new patch set (#5). Change subject: [java] log warning if applying operation on a closed session .. [java] log warning if applying operation on a closed session Closed sessions won't flush on another close() call, so it's somewhat dubious to apply() an operation to a closed session. This change adds a warning if apply() is used with a closed session. Originally this patch enforced the new behavior via precondition, but that's a breaking change for clients that relied on the old broken behavior. Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 2 files changed, 30 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/12237/5 -- To view, visit http://gerrit.cloudera.org:8080/12237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0fe221fedbb91959985f5ee374f1b691be2426a9 Gerrit-Change-Number: 12237 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc@2055
PS5, Line 2055: ASSERT_OK(ReopenBlockManager(entity));
Here you can use the FsReport to check that nothing was repaired. Same with
case 11.
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc@869
PS5, Line 869: if (!read_status.IsEndOfFile() &&
!read_status.IsIncomplete()) {
Should probably add a comment to explain this:
// If the read failed for some unexpected reason, propagate the error.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 5
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Wed, 23 Jan 2019 17:49:02 +
Gerrit-HasComments: Yes
[kudu-CR] python: fix Python 3.4 based tests
Adar Dembo has removed a vote on this change. Change subject: python: fix Python 3.4 based tests .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Iecbc55877e832a0ef437d2799e09c39fb314cb95 Gerrit-Change-Number: 12255 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] python: fix Python 3.4 based tests
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12255 ) Change subject: python: fix Python 3.4 based tests .. Patch Set 2: Verified+1 Overriding Jenkins. The build failures were due to thirdparty changes in Grant's Docker change (https://gerrit.cloudera.org/c/12173/), and since PS2 was a comment-only change and PS1 passed, it should build fine too. -- To view, visit http://gerrit.cloudera.org:8080/12255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iecbc55877e832a0ef437d2799e09c39fb314cb95 Gerrit-Change-Number: 12255 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 17:35:30 + Gerrit-HasComments: No
[kudu-CR] python: fix Python 3.4 based tests
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12255 ) Change subject: python: fix Python 3.4 based tests .. python: fix Python 3.4 based tests Pip released 19.0 recently and tests based on Python 3.4 started to fail. In addition, Python 3.4 will no longer be supported beginning with pip 19.1. Therefore, this patch updates the pip version to the last one that works. The full error message is below. $ pip install pandas DEPRECATION: Python 3.4 support has been deprecated. pip 19.1 will be the last one supporting it. Please upgrade your Python as Python 3.4 won't be maintained after March 2019 (cf PEP 429). Collecting pandas Using cached https://files.pythonhosted.org/packages/08/01/803834bc8a4e708aedebb133095a88a4dad9f45bbaf5ad777d2bea543c7e/pandas-0.22.0.tar.gz Installing build dependencies ... done Getting requirements to build wheel ... error Complete output from command /home/jenkins-slave/a/bin/python3 /home/jenkins-slave/a/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmpsknqc6o3: Traceback (most recent call last): File "/home/jenkins-slave/a/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py", line 207, in main() File "/home/jenkins-slave/a/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py", line 197, in main json_out['return_val'] = hook(**hook_input['kwargs']) File "/home/jenkins-slave/a/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py", line 54, in get_requires_for_build_wheel return hook(config_settings) File "/tmp/pip-build-env-z5ha9gs9/overlay/lib/python3.4/site-packages/setuptools/build_meta.py", line 115, in get_requires_for_build_wheel return _get_build_requires(config_settings, requirements=['wheel']) File "/tmp/pip-build-env-z5ha9gs9/overlay/lib/python3.4/site-packages/setuptools/build_meta.py", line 101, in _get_build_requires _run_setup() File "/tmp/pip-build-env-z5ha9gs9/overlay/lib/python3.4/site-packages/setuptools/build_meta.py", line 85, in _run_setup exec(compile(code, __file__, 'exec'), locals()) File "setup.py", line 27, in import versioneer ImportError: No module named 'versioneer' Change-Id: Iecbc55877e832a0ef437d2799e09c39fb314cb95 Reviewed-on: http://gerrit.cloudera.org:8080/12255 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M build-support/jenkins/build-and-test.sh 1 file changed, 8 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iecbc55877e832a0ef437d2799e09c39fb314cb95 Gerrit-Change-Number: 12255 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [docker] Add an initial docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 ) Change subject: [docker] Add an initial docker build .. Patch Set 3: If your build failed with exit code 137 it's likely not due to the build but the memory available to docker not being enough. See here: https://success.docker.com/article/what-causes-a-container-to-exit-with-code-137 -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 14:21:11 + Gerrit-HasComments: No
[kudu-CR] [docker] Add an initial docker build
Attila Bukor has posted comments on this change. (
http://gerrit.cloudera.org:8080/12173 )
Change subject: [docker] Add an initial docker build
..
Patch Set 3:
(3 comments)
I tried to build it on my Mac and kudu-thirdparty fails for some reason:
The command '/bin/sh -c thirdparty/build-if-necessary.sh && rm -rf
thirdparty/src' returned a non-zero code: 137
Chargers-MacBook-Pro:kudu charger$
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile
File docker/Dockerfile:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@32
PS3, Line 32: ARG BASE_OS=ubuntu:xenial
I'm curious, why do we default to ubuntu:xenial? In patch set one we used
centos7.
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh
File docker/docker-build.sh:
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@37
PS3, Line 37: --build-arg MAINTAINER="Apache Kudu"
nit: I think there should be a space between the name and the email
http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@45
PS3, Line 45: docker build "${BUILD_ARGS[@]}" -f $ROOT/docker/Dockerfile
--target kudu-base -t kudu-base $ROOT
I believe the tag should be kudu:* instead of kudu-* and each image should have
a tag with and one without the version, e.g.
"... -t kudu:thirdparty -t kudu:$VERSION-thirdparty kudu:latest-thirdparty
$ROOT"
We should probably tag the kudu-runtime image without the runtime part to serve
as a "default" image:
"... --target kudu-runtime -t kudu:latest -t kudu:$VERSION $ROOT"
Also I'm not convinced we need to tag kudu-thirdparty and kudu-build. It's good
that they're in separate stages and If anyone wants to build them for some
reason, they can still do it.
--
To view, visit http://gerrit.cloudera.org:8080/12173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42
Gerrit-Change-Number: 12173
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Jan 2019 12:19:31 +
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hello Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12239 to look at the new patch set (#5). Change subject: [fs]: delete the (orphaned) metadata file when repairing .. [fs]: delete the (orphaned) metadata file when repairing With KUDU-2636 fixed, we now support deleting dead containers at runtime. Because this involves deleting a pair of files, there's a time window in which it's possible for there to be just one file. A well-timed crash or kill -9 can make this a permanent state that fails the log block manager at startup. Indeed, a loop of dense_node-itest failed 2/1000 times in this way. This patch fixes the problem by detecting "orphaned" metadata files at startup and deleting them. A metadata file is orphaned if it has no corresponding data file and if it contains nothing but dead blocks. Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 2 files changed, 336 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/5 -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 5 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG@9 PS4, Line 9: This patch intends to delete the (orphaned) metadata file at : startup while the data file has gone missing and the metadata : file has no live blocks at the same time. In the previous patch : KUDU-2636, it has supported deleting the dead container which : includes data file and metadata file. So, there will be a time : window while deleting both of these two files in sequential, : and it will make a half-container in extreme cases, especially : in a crash or sent the tserver a kill -9. Indeed, Adar has : captured this type of case under 1000 runs of dense_node-itest. > Thanks for the added detail. Could you reword this a bit to add more clarit Done -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 4 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 23 Jan 2019 11:29:21 + Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
helifu has posted comments on this change. (
http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
..
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1896
PS4, Line 1896: const auto CheckRepaired = [&] () {
> If you really want to test that the repair happened, you should look at the
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1987
PS4, Line 1987: // Create a metadata file with 1 byte which is metadata_file_writer;
: ASSERT_OK(env_->NewWritableFile(metadata_file_name,
_file_writer));
: ASSERT_OK(metadata_file_writer->Append(Slice("a")));
: metadata_file_writer->Close();
> Since this pattern is repeated quite a few times, could you encapsulate it
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@2029
PS4, Line 2029: // Delete the only block.
: vector deleted;
: shared_ptr transaction =
bm_->NewDeletionTransaction();
: transaction->AddDeletedBlock(block_id);
: ASSERT_OK(transaction->CommitDeletedBlocks());
: transaction.reset();
: // Wait for the actual hole punching to take place.
: for (const auto& data_dir : dd_manager_->data_dirs()) {
: data_dir->WaitOnClosures();
: }
> Likewise, encapsulate this in a lambda.
Done
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774:
*-***-*-*/
> I don't think the order of which file is created/deleted matters because wi
Agreed:)
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: return Status::OK();
> If you look at all the filesystem calls made in this file, they're wrapped
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@762
PS4, Line 762: /* Note: the status here only represents the result of check.
> Could you reformat this to use multi-line C++ comments?
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@767
PS4, Line 767: EXIST && NO LIVE BLCOKS | EXIST && LIVE BLCOKS|
> BLOCKS
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@769
PS4, Line 769: (new case)
> Don't need this; if you really want to distinguish between the old and new
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@830
PS4, Line 830:
report->incomplete_container_check->entries.emplace_back(common_path);
: return Status::Aborted(Substitute("orphaned empty metadata
and data files $0", common_path));
> Nit: looks like these lines are indented too much.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@839
PS4, Line 839: if (PREDICT_FALSE(s_data.IsNotFound())) {
: if (metadata_size >=
pb_util::kPBContainerMinimumValidLength) {
> Can combine these and save one level of indentation.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@863
PS4, Line 863: if ((read_status.IsEndOfFile() && live_blocks.empty())) {
> Nit: there's an extra set of parens here.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@865
PS4, Line 865: return Status::Aborted(Substitute("orphaned metadata
file $0",common_path));
> Nit: need an extra space between the comma and 'common_path'
Done
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 4
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Wed, 23 Jan 2019 11:27:53 +
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
..
Patch Set 4:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG@9
PS4, Line 9: This patch intends to delete the (orphaned) metadata file at
: startup while the data file has gone missing and the metadata
: file has no live blocks at the same time. In the previous patch
: KUDU-2636, it has supported deleting the dead container which
: includes data file and metadata file. So, there will be a time
: window while deleting both of these two files in sequential,
: and it will make a half-container in extreme cases, especially
: in a crash or sent the tserver a kill -9. Indeed, Adar has
: captured this type of case under 1000 runs of dense_node-itest.
Thanks for the added detail. Could you reword this a bit to add more clarity?
With KUDU-2636 fixed, we now support deleting dead containers at runtime.
Because this involves deleting a pair of files, there's a time window in which
it's possible for there to be just one file. A well-timed crash or kill -9 can
make this a permanent state that fails the log block manager at startup.
Indeed,
a loop of dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files at startup
and deleting them. A metadata file is orphaned if it has no corresponding data
file and if it contains nothing but dead blocks.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1896
PS4, Line 1896: const auto CheckRepaired = [&] () {
If you really want to test that the repair happened, you should look at the
FsReport that's created when the block manager is opened, and test the
LBMIncompleteContainerCheck within it.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1987
PS4, Line 1987: // Create a metadata file with 1 byte which is metadata_file_writer;
: ASSERT_OK(env_->NewWritableFile(metadata_file_name,
_file_writer));
: ASSERT_OK(metadata_file_writer->Append(Slice("a")));
: metadata_file_writer->Close();
Since this pattern is repeated quite a few times, could you encapsulate it in a
lambda?
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@2029
PS4, Line 2029: // Delete the only block.
: vector deleted;
: shared_ptr transaction =
bm_->NewDeletionTransaction();
: transaction->AddDeletedBlock(block_id);
: ASSERT_OK(transaction->CommitDeletedBlocks());
: transaction.reset();
: // Wait for the actual hole punching to take place.
: for (const auto& data_dir : dd_manager_->data_dirs()) {
: data_dir->WaitOnClosures();
: }
Likewise, encapsulate this in a lambda.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774:
*-***-*-*/
> In addition, the metadata file is created before data file in 'LogBlockCont
I don't think the order of which file is created/deleted matters because
without an fsync() on the parent directory, the filesystem may reorder
operations under the hood. A great reference on this is
https://danluu.com/file-consistency (see the "Filesystem semantics" section);
_some_ filesystems may reorder directory operations, though among ext4/xfs (the
filesystems we generally support for Kudu) this doesn't appear to be a problem.
Since this doesn't appear to be a problem on ext4/xfs, we can cross this bridge
when it becomes an actual issue.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: return Status::OK();
> Our initial goal was to cover scenarios when the data file has gone missing
If you look at all the filesystem calls made in this file, they're wrapped in
macros like RETURN_NOT_OK_CONTAINER_DISK_FAILURE that capture failed I/O and
convert it into a disk failure. Because disks can fail at any time and for any
reason, I think it's important to ensure that this wrapping is 100% inclusive.
In your patch, if the disk fails during ReadNextPB, we won't notice that.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc
File
