[kudu-CR] [docs] fix spark integration examples
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13927 ) Change subject: [docs] fix spark integration examples .. Patch Set 1: I'm not familiar with spark, however, I have run to check these examples in my environment, it works OK. -- To view, visit http://gerrit.cloudera.org:8080/13927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a48cc34aea3cc42afd48f43e142a669081f14e2 Gerrit-Change-Number: 13927 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 04:06:13 + Gerrit-HasComments: No
[kudu-CR] [docs] fix spark integration examples
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13927 ) Change subject: [docs] fix spark integration examples .. Patch Set 1: preview; https://github.com/acelyc111/kudu/blob/fdf6acd7ee682bf74bf72c5eac2de04c4e6c962b/docs/developing.adoc -- To view, visit http://gerrit.cloudera.org:8080/13927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a48cc34aea3cc42afd48f43e142a669081f14e2 Gerrit-Change-Number: 13927 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 04:03:37 + Gerrit-HasComments: No
[kudu-CR] [docs] fix spark integration examples
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13927 Change subject: [docs] fix spark integration examples .. [docs] fix spark integration examples Change-Id: I2a48cc34aea3cc42afd48f43e142a669081f14e2 --- M docs/developing.adoc 1 file changed, 14 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/13927/1 -- To view, visit http://gerrit.cloudera.org:8080/13927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2a48cc34aea3cc42afd48f43e142a669081f14e2 Gerrit-Change-Number: 13927 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13864 ) Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Reviewed-on: http://gerrit.cloudera.org:8080/13864 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/client/client-test.cc M src/kudu/common/partial_row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 11 files changed, 441 insertions(+), 122 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 7 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] [test] handle ScanTableToStrings() result status
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/11825 ) Change subject: [test] handle ScanTableToStrings() result status .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/client/client-test.cc@3523 PS2, Line 3523: ASSERT_OK(ScanTableToStrings(client_table_.get(), )); Some other place use two ASSERT, compare with rows.size() and rows[0], could you unify them in the same style? http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/11825/2/src/kudu/integration-tests/alter_table-test.cc@694 PS2, Line 694: std::sort(rows->begin(), rows->end()); Seems many place will sort rows after ScanTableToStrings, how about wrap it into the function? -- To view, visit http://gerrit.cloudera.org:8080/11825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97a68f4c3ba5041848adddbefeb22e64b42a2745 Gerrit-Change-Number: 11825 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 00:15:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 ) Change subject: KUDU-2625: Support per-row error check in prepare stage .. Patch Set 6: Code-Review+2 Alexey do you want to take another look? -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 6 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 00:07:07 + Gerrit-HasComments: No
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 ) Change subject: KUDU-2625: Support per-row error check in prepare stage .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h@113 PS5, Line 113: Status ReadColumnAndDiscard(const ColumnSchema& col); > Nit: maybe call it ReadColumnAndDiscard instead? Done http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616 PS3, Line 616: ops->push_back(op); : } : > But the per-row errors still propagate through Apply and end up returned to Yes http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc@461 PS5, Line 461: if (op->has_result()) continue; > Just so I understand, this is an optimization in that we're not going to ap Yes -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 6 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 00:02:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13864 to look at the new patch set (#6). Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 --- M src/kudu/client/client-test.cc M src/kudu/common/partial_row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 11 files changed, 441 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/6 -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 6 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2163: consistently use dashes for gflag references in docs
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13924 ) Change subject: KUDU-2163: consistently use dashes for gflag references in docs .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Gerrit-Change-Number: 13924 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin
[kudu-CR] KUDU-2163: consistently use dashes for gflag references in docs
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13924 ) Change subject: KUDU-2163: consistently use dashes for gflag references in docs .. Patch Set 1: Verified+1 Overriding Jenkins, unrelated failure was in NTP. -- To view, visit http://gerrit.cloudera.org:8080/13924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Gerrit-Change-Number: 13924 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 23:15:10 + Gerrit-HasComments: No
[kudu-CR] KUDU-2163: consistently use dashes for gflag references in docs
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13924 ) Change subject: KUDU-2163: consistently use dashes for gflag references in docs .. KUDU-2163: consistently use dashes for gflag references in docs This chips away at KUDU-1984, but I'd rather have us use one style or the other consistently vs. somewhere in the middle. Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Reviewed-on: http://gerrit.cloudera.org:8080/13924 Reviewed-by: Alexey Serbin Tested-by: Adar Dembo --- M docs/design-docs/cfile.md M docs/prior_release_notes.adoc 2 files changed, 2 insertions(+), 2 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/13924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Gerrit-Change-Number: 13924 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin
[kudu-CR] KUDU-2163: consistently use dashes for gflag references in docs
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13924 ) Change subject: KUDU-2163: consistently use dashes for gflag references in docs .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Gerrit-Change-Number: 13924 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 23:08:14 + Gerrit-HasComments: No
[kudu-CR] KUDU-2163: consistently use dashes for gflag references in docs
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/13924 to review the following change. Change subject: KUDU-2163: consistently use dashes for gflag references in docs .. KUDU-2163: consistently use dashes for gflag references in docs This chips away at KUDU-1984, but I'd rather have us use one style or the other consistently vs. somewhere in the middle. Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 --- M docs/design-docs/cfile.md M docs/prior_release_notes.adoc 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/13924/1 -- To view, visit http://gerrit.cloudera.org:8080/13924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia62fc2658f99dcf18d2d4ab308552080ecb43ec0 Gerrit-Change-Number: 13924 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin
[kudu-CR] comment: delete the outdated comment
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13917 ) Change subject: comment: delete the outdated comment .. Patch Set 1: > > Patch Set 1: Code-Review+2 > > > > I couldn't find a document with this name anywhere in Kudu's > commit history. Perhaps it's something David forgot to merge and > has since been lost to time. > > > > Andrew, does this ring any bells to you? > > It doesn't. My best guess is this may have pointed to > https://github.com/apache/kudu/blob/37a81b82cc2e5e1ca9ad7291740b4e963e609bcc/docs/transaction_semantics.adoc#recommendations, > which outlines transaction semantics with respect to scans. > > Patch Set 1: Code-Review+2 > > > > I couldn't find a document with this name anywhere in Kudu's > commit history. Perhaps it's something David forgot to merge and > has since been lost to time. > > > > Andrew, does this ring any bells to you? > > It doesn't. My best guess is this may have pointed to > https://github.com/apache/kudu/blob/37a81b82cc2e5e1ca9ad7291740b4e963e609bcc/docs/transaction_semantics.adoc#recommendations, > which outlines transaction semantics with respect to scans. It seems the reference to docs/design-docs/repeatable-reads.md appeared in time_manager.h in the very first revision. Also, referencing transaction_semantics.adoc from mvcc.h mentions 'clean' time, which is not mentioned anywhere in the top-level of design docs (as I can see). Most likely, it might be a document that David had in his local git workspace, but it hasn't been committed? -- To view, visit http://gerrit.cloudera.org:8080/13917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Gerrit-Change-Number: 13917 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang Gerrit-Comment-Date: Thu, 25 Jul 2019 21:23:11 + Gerrit-HasComments: No
[kudu-CR] comment: delete the outdated comment
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13917 ) Change subject: comment: delete the outdated comment .. comment: delete the outdated comment Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Reviewed-on: http://gerrit.cloudera.org:8080/13917 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/consensus/time_manager.h M src/kudu/tablet/mvcc.h 2 files changed, 0 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Gerrit-Change-Number: 13917 Gerrit-PatchSet: 2 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang
[kudu-CR] comment: delete the outdated comment
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13917 ) Change subject: comment: delete the outdated comment .. Patch Set 1: > Patch Set 1: Code-Review+2 > > I couldn't find a document with this name anywhere in Kudu's commit history. > Perhaps it's something David forgot to merge and has since been lost to time. > > Andrew, does this ring any bells to you? It doesn't. My best guess is this may have pointed to https://github.com/apache/kudu/blob/37a81b82cc2e5e1ca9ad7291740b4e963e609bcc/docs/transaction_semantics.adoc#recommendations, which outlines transaction semantics with respect to scans. -- To view, visit http://gerrit.cloudera.org:8080/13917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Gerrit-Change-Number: 13917 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang Gerrit-Comment-Date: Thu, 25 Jul 2019 21:18:58 + Gerrit-HasComments: No
[kudu-CR] WIP [clock] introduce mini chronyd
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13916 )
Change subject: WIP [clock] introduce mini_chronyd
..
Patch Set 3:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h
File src/kudu/clock/test/mini_chronyd.h:
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@40
PS3, Line 40: // Default: 0
Nit: add an empty line between this and the previous line.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@53
PS3, Line 53: // Default: ""
What effect will this have on the running chronyd?
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@67
PS3, Line 67:
Should doc the defaults of the below options, and what effects the defaults
have.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@85
PS3, Line 85: the
Nit: drop 'the'
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@90
PS3, Line 90: // TODO(aserbin): how to run multiple chronyd processes for the
same minicluster
: //Introduce an index, and in case if not using
UNIQUE_LOOPBACK
: //it's also necessary to dedicated ports.
: //In the latter case, resrve the ports the same
way as it's done
: //for multi-master configurations.
Agreed with all of this. Follow-on work?
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@102
PS3, Line 102: // Creates a new MiniChronyd with the default options.
: MiniChronyd();
Maybe combine with the one-arg ctor?
explicit MiniChronyd(MiniChronydOptions options = MiniChronydOptions());
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@95
PS3, Line 95: if (!kudu_home) {
: return Status::ConfigurationError(
: "KUDU_HOME environment variable is not defined");
: }
This isn't typically defined. Seems like you should follow what
MiniSentry/MiniHMS do.
Same below.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@135
PS3, Line 135: // The chronyd's implementation puts strict requirements on
the ownership
: // of the directories where the runtime data is stored.
: RETURN_NOT_OK(CorrectOwnership(options_.data_root));
Yeah but would we actually expect data_root to have been created by someone
else? If the desired usage is tests, wouldn't the same user be responsible for
all of this?
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@151
PS3, Line 151: "-u", username,
The username needs to be specified on the command line and in the config file?
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@180
PS3, Line 180: RETURN_NOT_OK(proc->Kill(SIGTERM));
: return proc->Wait();
Want to use KillAndWait() here?
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@195
PS3, Line 195: LOG(INFO) << "SETTIME " << time_to_set;
Probably merits more context.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@220
PS3, Line 220: user $0
: bindaddress $1
: bindcmdaddress $2
: driftfile $3
: port $4
: dumpdir $5
: pidfile $6
Would also be nice to doc what these mean, so the config file is more readable.
--
To view, visit http://gerrit.cloudera.org:8080/13916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7
Gerrit-Change-Number: 13916
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Jul 2019 19:31:11 +
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] introduce chrony
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13915 ) Change subject: [thirdparty] introduce chrony .. Patch Set 1: (4 comments) How long does chrony take to build? http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/build-definitions.sh@1013 PS1, Line 1013: #DESTDIR=$PREFIX \ Remove? http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/build-definitions.sh@1019 PS1, Line 1019: ./configure \ Maybe add a comment rationalizing these config options? http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/build-thirdparty.sh@577 PS1, Line 577: if [ -n "$F_TSAN" -o -n "$F_CHRONY" ]; then : build_chrony : fi If we're only using it to test, could we put it in F_COMMON instead of F_UNINSTRUMENTED? http://gerrit.cloudera.org:8080/#/c/13915/1/thirdparty/patches/chrony-no-superuser.patch File thirdparty/patches/chrony-no-superuser.patch: PS1: Could you structure the patch as a git commit against master (or 3.5 or whatever)? That way there's more useful metadata that can be used to track provenance after your patch is accepted upstream. -- To view, visit http://gerrit.cloudera.org:8080/13915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71ed12311b10979af8a12094881b6b8b47ef8008 Gerrit-Change-Number: 13915 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 19:16:10 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
..
Patch Set 12:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@98
PS12, Line 98: lower
upper
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@484
PS12, Line 484: Status ConvertToKuduPartialRow(const
client::sp::shared_ptr& table,
Could some of TableScanner be reused for this? There's logic there to
deserialize JSON into predicates, which seems quite similar.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@592
PS12, Line 592: case KuduColumnSchema::BOOL: {
: bool value;
: RETURN_NOT_OK_PREPEND(
: reader.ExtractBool(values[i], /*field=*/nullptr,
),
: Substitute(kErrorMsgArg,
:values[i],
:col_name,
:
KuduColumnSchema::DataTypeToString(type)));
: range_bound_partial_row->SetBool(col_name, value);
: break;
: }
> Per https://kudu.apache.org/docs/schema_design.html#primary-keys, boolean
Agreed with Andrew, though in this case I wouldn't use FALLTHROUGH_INTENDED,
because we generally only use it when the missing 'break' looks like a mistake.
For example:
switch (foo) {
case A:
DoA();
break;
case B:
DoB();
case C:
default:
DoDefault();
break;
When I read this, I notice that case B does something but doesn't have a
'break', which looks like an error. I also notice that case C chains directly
into the default case, which seems normal. If the intent for case B was to call
DoB() and DoDefault(), FALLTHROUGH_INTENDED is appropriate:
switch (foo) {
case A:
DoA();
break;
case B:
DoB();
FALLTHROUGH_INTENDED;
case C:
default:
DoDefault();
break;
Under the hood, FALLTHROUGH_INTENDED expands to the [[clang::fallthrough]]
attribute, which affects compilation with -Wimplicit-fallthrough. Of note: when
compiling a "direct chain" with -Wimplicit-fallthrough but without
[[clang::fallthrough]], no warning is generated. This further emphasizes that
FALLTHROUGH_INTENDED is only needed for non-obvious chaining. See
https://clang.llvm.org/docs/AttributeReference.html#fallthrough for details.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@649
PS12, Line 649: if (!boost::iequals(FLAGS_lower_bound_type,
"INCLUSIVE_BOUND") &&
: !boost::iequals(FLAGS_lower_bound_type,
"EXCLUSIVE_BOUND")) {
: return Status::InvalidArgument(
: "wrong type of range lower bound : only
'INCLUSIVE_BOUND' or "
: "'EXCLUSIVE_BOUND' can be received");
: }
: if (!boost::iequals(FLAGS_upper_bound_type,
"INCLUSIVE_BOUND") &&
: !boost::iequals(FLAGS_upper_bound_type,
"EXCLUSIVE_BOUND")) {
: return Status::InvalidArgument(
: "wrong type of range upper bound : only
'INCLUSIVE_BOUND' or "
: "'EXCLUSIVE_BOUND' can be received");
: }
Can use a lambda here to deduplicate.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@675
PS12, Line 675: boost::iequals(FLAGS_lower_bound_type, "INCLUSIVE_BOUND") ?
: KuduTableCreator::INCLUSIVE_BOUND :
: KuduTableCreator::EXCLUSIVE_BOUND;
: KuduTableCreator::RangePartitionBound upper_bound_type =
: boost::iequals(FLAGS_upper_bound_type, "EXCLUSIVE_BOUND") ?
: KuduTableCreator::EXCLUSIVE_BOUND :
: KuduTableCreator::INCLUSIVE_BOUND;
> nit: per the style guide, indent at 4 spaces for multi-line statements.
I'd combine this in with the validation on L649-660. You can imagine a lambda
(or helper function) that returns a Status and writes a
KuduTableCreator::RangePartitionBound OUT parameter.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@690
PS12, Line 690:
Nit: DCHECK_EQ(PartitionAction::DROP, action);
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@843
PS12, Line 843: lower
upper
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@846
PS12, Line 846: lower
upper
--
To view, visit http://gerrit.cloudera.org:8080/13833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch:
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 ) Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: PS3: > > Doesn't this change mean that old servers (those that don't support In general we preserve backwards compatibility for "both" combinations (i.e. "new" server with "old" client and "old" server with "new" client). That's why if e.g. we decide to replace one RPC with another, you'll find server-side code that handles both RPCs, and client-side code that looks at server capabilities to decide which RPC to send. There are exceptions where we've failed to do this, but this is the general policy for Kudu. -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Thu, 25 Jul 2019 18:47:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 ) Change subject: KUDU-2625: Support per-row error check in prepare stage .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h@113 PS5, Line 113: Status ConsumeUselessColumn(const ColumnSchema& col); Nit: maybe call it ReadColumnAndDiscard instead? http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616 PS3, Line 616: ops->push_back(op); : } : > I've removed these code, since per-row errors not supported when return fro But the per-row errors still propagate through Apply and end up returned to the client, right? http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc@461 PS5, Line 461: if (op->has_result()) continue; Just so I understand, this is an optimization in that we're not going to apply that op (because it failed), so there's no reason to take a lock on its primary key? -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 25 Jul 2019 18:43:52 + Gerrit-HasComments: Yes
[kudu-CR] comment: delete the outdated comment
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13917 ) Change subject: comment: delete the outdated comment .. Patch Set 1: Code-Review+2 I couldn't find a document with this name anywhere in Kudu's commit history. Perhaps it's something David forgot to merge and has since been lost to time. Andrew, does this ring any bells to you? -- To view, visit http://gerrit.cloudera.org:8080/13917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Gerrit-Change-Number: 13917 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang Gerrit-Comment-Date: Thu, 25 Jul 2019 18:12:05 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] introduce chrony
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13915 ) Change subject: [thirdparty] introduce chrony .. Patch Set 1: That makes sense. Thanks for clarifying. -- To view, visit http://gerrit.cloudera.org:8080/13915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71ed12311b10979af8a12094881b6b8b47ef8008 Gerrit-Change-Number: 13915 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 17:51:26 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] introduce chrony
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13915 ) Change subject: [thirdparty] introduce chrony .. Patch Set 1: > Given chrony is gplv2 licensed are we able to include it in our > build/project? See the Apache summary here: > https://www.apache.org/licenses/GPL-compatibility.html The use of chrony is only as a part of testing framework for Kudu: we are not linking it in or depending on it such a way that it might be considered as a sort of derivative project. FWIW, we use bison in 3rd-party as well it's under GPLv3, so I anticipate that GPLv2 license for a test-only tool shouldn't be a problem if it's not a problem for including bison which is build-time dependency and is under GPLv3. -- To view, visit http://gerrit.cloudera.org:8080/13915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71ed12311b10979af8a12094881b6b8b47ef8008 Gerrit-Change-Number: 13915 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 17:00:53 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] introduce chrony
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13915 ) Change subject: [thirdparty] introduce chrony .. Patch Set 1: Given chrony is gplv2 licensed are we able to include it in our build/project? See the Apache summary here: https://www.apache.org/licenses/GPL-compatibility.html -- To view, visit http://gerrit.cloudera.org:8080/13915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71ed12311b10979af8a12094881b6b8b47ef8008 Gerrit-Change-Number: 13915 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 14:04:38 + Gerrit-HasComments: No
[kudu-CR] [examples] Add a complete Nifi quickstart example
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13878 ) Change subject: [examples] Add a complete Nifi quickstart example .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13878/4/examples/quickstart/nifi/README.adoc File examples/quickstart/nifi/README.adoc: http://gerrit.cloudera.org:8080/#/c/13878/4/examples/quickstart/nifi/README.adoc@92 PS4, Line 92: value > nit: field? Done http://gerrit.cloudera.org:8080/#/c/13878/4/examples/quickstart/nifi/README.adoc@100 PS4, Line 100: r Kudu` template to the canvas. > We chatted about this on Slack, but we need to also manually enable the con I added a section on starting controller services. http://gerrit.cloudera.org:8080/#/c/13878/4/examples/quickstart/nifi/README.adoc@101 PS4, Line 101: : Once the template is added to the canvas you can start each processor by : right-clicking each processor and selecting `Start`. You can also explore : the configuration, queue contents, and more by right-clicking on each element. > You can also click Start for all the processes en masse by clicking Start f I noted this as an alternative. -- To view, visit http://gerrit.cloudera.org:8080/13878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71f3bc5898c15d7bc19cffb3a91b9efac3f6928b Gerrit-Change-Number: 13878 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 13:42:37 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add a complete Nifi quickstart example
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13878 to look at the new patch set (#5). Change subject: [examples] Add a complete Nifi quickstart example .. [examples] Add a complete Nifi quickstart example This patchs adds a brief example using Apache Nifi to ingest data into Apache Kudu. Change-Id: I71f3bc5898c15d7bc19cffb3a91b9efac3f6928b --- M docs/quickstart.adoc A examples/quickstart/nifi/README.adoc A examples/quickstart/nifi/Random_User_Kudu.xml M examples/quickstart/spark/README.adoc 4 files changed, 1,174 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/13878/5 -- To view, visit http://gerrit.cloudera.org:8080/13878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I71f3bc5898c15d7bc19cffb3a91b9efac3f6928b Gerrit-Change-Number: 13878 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13864 to look at the new patch set (#5). Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 --- M src/kudu/client/client-test.cc M src/kudu/common/partial_row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 11 files changed, 441 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/5 -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 ) Change subject: KUDU-2625: Support per-row error check in prepare stage .. Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2732 PS3, Line 2732: ASSERT_EQ(error->status().ToString(), : "Invalid argument: No fields updated, key is: (int32 key=1)"); > These lines look too long, could you wrap? Done http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2789 PS3, Line 2789: ASSERT_OK(ApplyInsertToSession(session.get(), client_table_, 1, 1, "One")); : ASSERT_OK(ApplyInsertToSess > Use ASSERTs here. Done http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2798 PS3, Line 2798: ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred"); > Need to wrap these calls in NO_FATALS. Done http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@449 PS3, Line 449: // etc. > Looking over DecodeUpdateOrDelete, it looks like the "nothing to consume, m I've wrap some code in ConsumeUselessColumn, the function name is self-explanatory, and I've added some comments in function declare place. It's not needed to add multi-row tests, because when a row doesn't properly validated (lack some data or left some data), DecodeOperations will return another error, we can detect this error in single row test in row_operations-test. http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@527 PS3, Line 527: op->SetFailureStatusOnce(Status::InvalidArgument( > Wrap in RETURN_NOT_OK? Or should this be wrapped in ignore_result? should be wrapped in RETURN_NOT_OK, Done. http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616 PS3, Line 616: ops->push_back(op); : } : > Why this short-circuit? Shouldn't we surface a batch of 10/10 decoding erro I've removed these code, since per-row errors not supported when return from prepare phase. Maybe I will add it in another patch. http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc File src/kudu/tablet/transactions/write_transaction.cc: http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@169 PS3, Line 169: // TODO(unknown): is MISMATCHED_SCHEMA always right here? probably not. > Is this here just to accommodate the new "if all ops failed to decode, fail removed. http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@220 PS3, Line 220: } > Nit: not your fault (since you're just moving code), but this would probabl Done -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 25 Jul 2019 09:13:56 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13864 to look at the new patch set (#4). Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 --- M src/kudu/client/client-test.cc M src/kudu/common/partial_row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 11 files changed, 441 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/4 -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] comment: delete the outdated comment
XiaokaiWang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13917 Change subject: comment: delete the outdated comment .. comment: delete the outdated comment Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 --- M src/kudu/consensus/time_manager.h M src/kudu/tablet/mvcc.h 2 files changed, 0 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13917/1 -- To view, visit http://gerrit.cloudera.org:8080/13917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If6f1a73c19ccb250f02f3faf9993521de3a842b5 Gerrit-Change-Number: 13917 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 ) Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@250 PS3, Line 250: const string& ta > Capture by cref if you're not using std::move. Done http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@275 PS3, Line 275: NO_FATALS(get_tablet_location(proxy_.get(), loc.tablet_id())); > Need to wrap in NO_FATALS. Done http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto@a565 PS3, Line 565: > I think this comment was useful; could you restore it, and duplicate it int Done -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Thu, 25 Jul 2019 07:20:12 + Gerrit-HasComments: Yes
[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13908 to look at the new patch set (#4). Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client .. [client] Deprecated TabletLocationsPB.ReplicaPB message in client According to the comments of https://gerrit.cloudera.org/#/c/13875/, the TabletLocationsPB.ReplicaPB message has been deprecated a long time ago. In this patch, I removed the dependency on it from the client. The server-side is compatible with both old and new client. Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/linked_list-test.cc M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/integration-tests/raft_config_change-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/rebalancer_tool-test.cc 27 files changed, 272 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/4 -- To view, visit http://gerrit.cloudera.org:8080/13908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef Gerrit-Change-Number: 13908 Gerrit-PatchSet: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
..
Patch Set 12:
(8 comments)
This is looking good! Most of my comments are pointing out potential code reuse
in the tests.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2357
PS12, Line 2357: // Lambda function to insert test rows into ktableId,
: // the key value is pass by parameter, the other two columns'
: // value are specified.
: auto insert_test_row = [&] (int32_t value) -> Status {
: unique_ptr insert(table->NewInsert());
: auto* row = insert->mutable_row();
: RETURN_NOT_OK(row->SetInt32("key", value));
: RETURN_NOT_OK(row->SetInt32("int_val", 12345));
: RETURN_NOT_OK(row->SetString("string_val", "hello"));
: auto session = client_->NewSession();
: RETURN_NOT_OK(session->Apply(insert.release()));
: RETURN_NOT_OK(session->Flush());
: RETURN_NOT_OK(session->Close());
: return Status::OK();
: };
This seems pretty similar to insert_test_rows(), defined in the next test. How
about defining this into its own function in an anonymous namespace, and then
reusing it in the next test case too. Something like:
Status InsertTestRows(int start_key, int num_rows) {
...
}
I'd also consider using the same schema in the next test case so we can reuse
it without worrying about schema differences, and it shouldn't change the
functionality of TestAddAndDropRangePartition.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2401
PS12, Line 2401: s = RunKuduTool({
: "table",
: "add_range_partition",
: master_addr,
: kTableId,
: "[]",
: "[]",
: }, , );
ASSERT_OK() here?
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2461
PS12, Line 2461: RETURN_NOT_OK(session->Apply(insert.release()));
How about we catch any errors here, and "unwrap" the pending errors and return
the first error? Basically, have this function return the first per-row error.
That way, we could reuse this function in insert_out_of_range_row().
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2668
PS12, Line 2668: {
: // Test adding (EXCLUSIVE_BOUND, UNBOUNDED) range
partition.
:
: // Adding (0,unbouded), lower range bound is 0, upper
range bound is unbounded,
: // 0 is exclusive.
: ASSERT_OK(add_range_partition_using_CLI("[0]", "[]",
"-lower_bound_type=EXCLUSIVE_BOUND",
:
"-upper_bound_type=EXCLUSIVE_BOUND"));
:
: // Insert 1000 rows from line_1 to line_1000, now there
are 1000 rows,
: // data range is: [1-1000].
: ASSERT_OK(client_->OpenTable(kTestTableName, ));
: ASSERT_OK(insert_test_rows(1, 1000));
: ASSERT_EQ(1000, CountTableRows(table.get()));
:
: // Test insert out of range rows, which will return error
in lambda as we expect.
: // Since the upper range bound is unbouded, there is no
num out of range as well
: // as it greater than 0.
: ASSERT_OK(insert_out_of_range_row(0));
:
: // Drop range partition of (0,unbouded) by command line,
now there are 0 rows left.
: ASSERT_OK(drop_range_partition_using_CLI("[0]", "[]",
"-lower_bound_type=EXCLUSIVE_BOUND",
:
"-upper_bound_type=EXCLUSIVE_BOUND"));
: ASSERT_OK(client_->OpenTable(kTestTableName, ));
: ASSERT_EQ(0, CountTableRows(table.get()));
: }
This same pattern is repeated many times. Maybe wrap this in a lambda like:
const auto check_bounds = [&] (const string& lower_bound, const string&
upper_bound,
const string& lower_bound_type, const string& upper_bound_type,
int start_row_to_insert, int num_rows_to_insert, vector
out_or_range_rows_to_insert) {
// Adds a partition, inserts some rows in that partition, inserts rows
outside that partition, drops the partition, and verifies no rows are left.
...
}
