Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18835 )
Change subject: KUDU-3353 [schema] Add an immutable attribute on column schema (part 2) ...................................................................... Patch Set 14: (26 comments) http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test-util.h File src/kudu/client/client-test-util.h: http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test-util.h@45 PS12, Line 45: LogSessio > >From what I can see, LogSessionErrorsAndDie() doesn't have any ASSERT_XXX o Removed it, I forgot to revert it when I ever changed LogSessionErrorsAndDie. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@246 PS12, Line 246: ->De > nit: once this becomes a method (i.e. it's not a constructor anymore), mayb Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@417 PS12, Line 417: void InsertTestRows(KuduTab > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@506 PS12, Line 506: > nit for here and other methods that became non-static after introduction of Reasonable, and I also make some other functions as const. Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@3076 PS12, Line 3076: ; > Why is it essential to have KuduSession::MANUAL_FLUSH instead of KuduSessio It's related to a previous implemention, I've reverted it to AUTO_FLUSH_SYNC and removed Flush(). http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8853 PS12, Line 8853: : INSTANTIATE_TEST_SUITE_P(Params, ClientTestImmutableColumn, > nit: maybe, create a constant out of this string and use it here and at lin Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8865 PS12, Line 8865: ASSERT_OK(session->Apply(insert.release())); > Could you also add a sub-scenario where UPDATE tries to set the immutable c Thank you to point it out, and I found a bug when set it to NULL. Fixed. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8894 PS12, Line 8894: client_table_->NewUpdate( > As I can see, the ClientTestImmutableColumn.TestUpdate scenario uses KuduSe Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8905 PS12, Line 8905: ERT_TRUE(s.IsIOError()) << s.ToString(); > nit: UPDATE IGNORE can update a row without changing the immutable column c Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8906 PS12, Line 8906: ERT_STR_CONTAINS > the immutable column Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8912 PS12, Line 8912: session->GetPendingErrors(&errors, &overflow); > Could you also add a sub-scenario where UPDATE IGNORE tries to set the immu Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8922 PS12, Line 8922: ScanToStrings(&scanner, &rows)); : ASSERT_EQ(1, rows.size()); > nit: make a constant and use it here and at line 8953 as well? Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8931 PS12, Line 8931: ASSERT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_SYNC)); > Could you also add a sub-scenario where UPDATE IGNORE tries to set only the Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8996 PS12, Line 8996: ASSERT_EQ(1, rows.size()); > Could you also add a sub-scenario where UPSERT IGNORE tries to set the immu Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9012 PS12, Line 9012: > How significant MANUAL_FLUSH vs AUTO_FLUSH_SYNC here? Please add a comment Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9023 PS12, Line 9023: : vector<string> rows; > nit: use a constant here and at line 9043? Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9029 PS12, Line 9029: "int32 non_null_with_default=999, int32 imm_val=$0)", > Could you also add a sub-scenario where UPSERT tries to set the immutable c Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9053 PS12, Line 9053: ASSERT_EQ(1, rows.size()); > In addition to these nice test scenarios, could you also add a few where th Sure, added TEST_SUITE_P for this. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9084 PS12, Line 9084: "failed to flush data: error details are available " > nit: add ' << s.ToString()' Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9108 PS12, Line 9108: FLAGS_master_support_immutable_column_attribute = false; > nit: add ' << s.ToString()' Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9113 PS12, Line 9113: InternalMiniClusterOptions options; > Could you also add a couple of test scenarios to verify that: Thanks for your suggestion, and it help me to find some small bugs. Done. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/ops/write_op.cc@514 PS12, Line 514: attempt > attempting Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/ops/write_op.cc@529 PS12, Line 529: attempt > attempting Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/row_op.h File src/kudu/tablet/row_op.h: http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/row_op.h@106 PS12, Line 106: bool failed = false; > This looks a bit strange to set 'completed' true straight off the start in Good idea! Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/tablet_metrics.cc@50 PS12, Line 50: will be > Why 'could be'? It would be great to have a clear criteria here what happe Done http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/tablet_metrics.cc@62 PS12, Line 62: re updat > ditto Done -- To view, visit http://gerrit.cloudera.org:8080/18835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id301e313eedd9cc3a494d6b4f3c14fbabe63b436 Gerrit-Change-Number: 18835 Gerrit-PatchSet: 14 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 09 Sep 2022 11:50:48 +0000 Gerrit-HasComments: Yes
