Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 19: (16 comments) 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/19097/19/src/kudu/client/client-test.cc@9924 PS19, Line 9924: blow column specs > column specs below Done 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 Done 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', 'sec Done 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 th Done http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/integration-tests/auto_incrementing-itest.cc@108 PS18, Line 108: > change to return. Done http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/integration-tests/auto_incrementing-itest.cc@124 PS18, Line 124: > return OK after finishing the loop Done http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/integration-tests/auto_incrementing-itest.cc@128 PS18, Line 128: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/integration-tests/auto_incrementing-itest.cc@142 PS18, Line 142: } > missing return value Done 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@125 PS19, Line 125: // Returns a scan response from the tablet on the given tablet server. : Status ScanTablet(int ts, const string& tablet_id, vector<string>* results) { : ScanResponsePB resp; : RpcController rpc; : ScanRequestPB req; : : NewScanRequestPB* scan = req.mutable_new_scan_request(); : scan->set_tablet_id(tablet_id); : scan->set_read_mode(READ_LATEST); : : Schema schema = Schema({ ColumnSchema("c0", INT32), : ColumnSchema("c1",INT64, false,false, true), : },2); : RETURN_NOT_OK(SchemaToColumnPBs(schema, scan->mutable_projected_columns())); : RETURN_NOT_OK(cluster_->tserver_proxy(ts)->Scan(req, &resp, &rpc)); : StringifyRowsFromResponse(schema, rpc, &resp, results); : return Status::OK(); : } > nit: indent space Done 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 Right, thanks! http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/19097/18/src/kudu/tablet/ops/write_op.cc@195 PS18, Line 195: if (state_->consensus_round()) { : if (is_leader && state_->tablet_replica()->tablet()->schema()->has_auto_incrementing()) { > Curious why need two if? Could we just move the position for state_->consen Yes, that makes sense. Done 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 o Yes, that is correct. Thanks for pointing that out. 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 Done 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 Done -- 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 23:15:25 +0000 Gerrit-HasComments: Yes
