Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 19: (11 comments) http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/16/src/kudu/client/client-test.cc@9959 PS16, Line 9959: CountDownLatch second_client_latch(1); > Yes, once we have the initial server and client bits in place we can have a OK, sounds good to me. Thanks! http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9860 PS19, Line 9860: blow column specs column specs below http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9882 PS19, Line 9882: .schema(&schema_) : .set_range_partition_columns({"key"}) : .add_range_partition(lower0.release(), upper0.release()) : .add_range_partition(lower1.release(), upper1.release()) : .num_replicas(3) : .Create()); nit for here and elsewhere in the new code below: the indent if off http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9924 PS19, Line 9924: blow column specs column specs below http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9931 PS19, Line 9931: int lower_bound = 0; : int upper_bound = 20000; nit: these are used only in one place, so maybe avoid adding a variable and use the constants as they are at lines 9936 and 9937 correspondingly? http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9949 PS19, Line 9949: shared_ptr<KuduClient> first_client; Could you use containers (say, a vector) instead of those 'first_xxx', 'second_xxx' variables? It could also help to reduce the amount of boiler plate code since then you'd be able to use 'for()' cycle to perform per-element actions. Also, why not to have, say, 8 clients? Contemporary CPUs are pretty much 8+ CPU cores in recent times. Two clients are OK, but I guess if there is any concurrency issue, then with more clients it could easier to expose it. http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@10010 PS19, Line 10010: ASSERT_TRUE(rows[0] == rows[1]); : ASSERT_TRUE(rows[1] == rows[2]); Could this be written as a cycle as well with SCOPED_TRACE since we know the number of elements in the 'rows' container? http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/integration-tests/auto_incrementing-itest.cc@153 PS19, Line 153: NO_FATALS Since you have chosen the second option, CreateTableWithData() now returns Status and that commands using ASSERT_OK() here instead of NO_FATALS(). Same for the rest of functions/methods below that now return 'Status' instead of 'void'. http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/tablet/ops/write_op.cc@197 PS19, Line 197: auto write_req = state_->consensus_round()->replicate_msg()->write_request(); : write_req.mutable_auto_incrementing_column()->set_auto_incrementing_counter( : state_->tablet_replica()->tablet()->GetAutoIncrementingCounter()); I might be mistaken, but this looks like setting a field into a temporary object 'write_req' which is a copy of the write request in ReplicateMsg. Should there be a call to 'mutable_write_request()' instead of 'write_request()'? http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/tablet/tablet.cc@618 PS19, Line 618: // If this is a follower replica, update the auto-incrementing value from the leader : // replica received in the raft consensus replicate message. Why not to do this in the call site of DecodeWriteOperations in write_op.cc instead? http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/tablet/tablet.cc@621 PS19, Line 621: op_state->consensus_round()->replicate_msg()-> : write_request() nit: it could be more readable if a ref variable were created for this and used in the code around -- To view, visit http://gerrit.cloudera.org:8080/19097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dbde9095da78f6d1bd00adcc0a6e7dd63082bbc Gerrit-Change-Number: 19097 Gerrit-PatchSet: 19 Gerrit-Owner: Abhishek Chennaka <[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: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 12 Jan 2023 03:00:47 +0000 Gerrit-HasComments: Yes
