Alexey Serbin 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 12: (28 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: NO_FATALS >From what I can see, LogSessionErrorsAndDie() doesn't have any ASSERT_XXX or >EXPECT_XXX, only CHECK() macros. Why to add NO_FATALS() then -- would be >great to add a comment to clarify (or just remove NO_FATALS() if it doesn't >provide any useful functionality here)? http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS4: > 1. Good idea! Tests of creating and altering table with immutable attribute I guess making Kudu client to require corresponding feature flag when sending UPSERT_IGNORE operations is a sort of overkill, but that would be possible. I'm just curious whether the client gets proper error (i.e. clear and actionable one) if the server doesn't support UPSERT_IGNORE. 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: CHECK_OK nit: once this becomes a method (i.e. it's not a constructor anymore), maybe convert this into 'return' and make BuildSchema() return Status? It makes sense to prefer ASSERT_OK() over CHECK_OK() if possible. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@417 PS12, Line 417: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@506 PS12, Line 506: virtual void PopulateDefaultRow(KuduPartialRow* row, int index) { nit for here and other methods that became non-static after introduction of the virtual PopulateDefaultRow() method: is it possible to make those methods 'const' (so far it seems they don't modify any member fields)? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@3076 PS12, Line 3076: KuduSession::MANUAL_FLUSH Why is it essential to have KuduSession::MANUAL_FLUSH instead of KuduSession::AUTO_FLUSH_SYNC here? Would be nice to add a comment to clarify on that. Otherwise, maybe keep KuduSession::AUTO_FLUSH_SYNC and remove KuduSession::Flush() calls in this scenario? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8853 PS12, Line 8853: "(int32 key=1, int32 int_val=999, string string_val=\"hello world\", " : "int32 non_null_with_default=999, int32 imm_val=4)" nit: maybe, create a constant out of this string and use it here and at line 8886 as well? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8865 PS12, Line 8865: ASSERT_OK(update->mutable_row()->SetInt32("imm_val", 888)); Could you also add a sub-scenario where UPDATE tries to set the immutable column cell to NULL? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8894 PS12, Line 8894: KuduSession::MANUAL_FLUSH As I can see, the ClientTestImmutableColumn.TestUpdate scenario uses KuduSession::AUTO_FLUSH_SYNC mode. How significant to have MANUAL_FLUSH mode here instead of AUTO_FLUSH_SYNC? Could you add a comment to clarify on this (if that's indeed significant)? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8905 PS12, Line 8905: UPDATE IGNORE can update row with immutable column drop nit: UPDATE IGNORE can update a row without changing the immutable column cell http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8906 PS12, Line 8906: immutable column the immutable column http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8912 PS12, Line 8912: ASSERT_OK(update_ignore->mutable_row()->SetInt32("imm_val", 888)); Could you also add a sub-scenario where UPDATE IGNORE tries to set the immutable column cell to NULL, please? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8922 PS12, Line 8922: "(int32 key=1, int32 int_val=888, string string_val=\"world hello\", " : "int32 non_null_with_default=888, int32 imm_val=4)" nit: make a constant and use it here and at line 8953 as well? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8931 PS12, Line 8931: ASSERT_OK(update_ignore->mutable_row()->SetInt32("imm_val", 888)); Could you also add a sub-scenario where UPDATE IGNORE tries to set only the immutable column cell to NULL? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@8996 PS12, Line 8996: ASSERT_OK(upsert_ignore->mutable_row()->SetInt32("imm_val", 888)); Could you also add a sub-scenario where UPSERT IGNORE tries to set the immutable column cell to NULL? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9012 PS12, Line 9012: MANUAL_FLUSH How significant MANUAL_FLUSH vs AUTO_FLUSH_SYNC here? Please add a comment to clarify. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9023 PS12, Line 9023: R"((int32 key=1, int32 int_val=1, string string_val="original row", )" : "int32 non_null_with_default=12345, int32 imm_val=1)" nit: use a constant here and at line 9043? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9029 PS12, Line 9029: ASSERT_OK(ApplyUpsertToSession(session.get(), client_table_, 1, 4, "upserted row 3", 999)); Could you also add a sub-scenario where UPSERT tries to set the immutable column cell to NULL? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9053 PS12, Line 9053: In addition to these nice test scenarios, could you also add a few where the immutable column cell is originally set to NULL during first insert/upsert, and then try to update that immutable NULL cell to a non-NULL value? http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9084 PS12, Line 9084: ASSERT_TRUE(s.IsNotSupported()); nit: add ' << s.ToString()' http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9108 PS12, Line 9108: ASSERT_TRUE(s.IsNotSupported()); nit: add ' << s.ToString()' http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/client/client-test.cc@9113 PS12, Line 9113: Could you also add a couple of test scenarios to verify that: * it's possible to alter a table adding 'immutable' attribute to a regular column, and after that writing and reading from the table works as expected * it's possible to alter a table removing the 'immutable' attribute from a column, and after that writing and reading the data from the table works as expected Thanks! http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/table_alterer-internal.cc@131 PS4, Line 131: s.spec->data_->primary_k > (a) Oh, sorry about that, I foget to remove it :( If not limiting table alterations that involve immutable columns, the server-side check isn't needed then, I guess :) 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 http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/ops/write_op.cc@529 PS12, Line 529: attempt attempting 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 completed = true; This looks a bit strange to set 'completed' true straight off the start in the constructor :) Maybe, instead call this 'failed' and set it only if the operation fails at some point? 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: could be Why 'could be'? It would be great to have a clear criteria here what happens with the 'rows_upserted' counter. http://gerrit.cloudera.org:8080/#/c/18835/12/src/kudu/tablet/tablet_metrics.cc@62 PS12, Line 62: could be ditto -- 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: 12 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: Thu, 08 Sep 2022 22:16:08 +0000 Gerrit-HasComments: Yes
