Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19650 )
Change subject: [rowset_metadata] update min/max encoded keys during bootstrapping ...................................................................... Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/19650/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19650/5//COMMIT_MSG@10 PS5, Line 10: initializing initialize http://gerrit.cloudera.org:8080/#/c/19650/5//COMMIT_MSG@21 PS5, Line 21: case test case http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tablet/cfile_set.cc@174 PS5, Line 174: // Update the min/max key in metadata explicitly if user specified. : if (FLAGS_rowset_metadata_store_keys) { : rowset_metadata_->set_min_encoded_key(min_encoded_key_); : rowset_metadata_->set_max_encoded_key(max_encoded_key_); : } Does it make sense to do so after verifying the validity of the keys? In other words, consider moving this to be after the check below, to be just before 'return Status::OK()'. http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4615 PS5, Line 4615: TestSetEncodedKeysWhenStartup nit: consider renaming into SetEncodedKeysUponStartup or SetEncodedKeysWhenStartingUp http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4618 PS5, Line 4618: when do restarting when restarting http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4628 PS5, Line 4628: key is not exist keys do not exist http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4630 PS5, Line 4630: RowSetDataPB* rowset_pb = superblock_pb.mutable_rowsets(rowset_no); nit: would it be enough to access just constant rowsets(rowset_no) field without requiring access to the mutable field? http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4634 PS5, Line 4634: Use Used http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4669 PS5, Line 4669: RowSetDataPB* rowset_pb = superblock_pb.mutable_rowsets(rowset_no); ditto for considering using accessor for immutable field instead http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4677 PS5, Line 4677: There is no encoded key exist without either 'There is no encoded key without ...' or 'The encoded key doesn't exist without ...' http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4685 PS5, Line 4685: we set the --rowset_metadata_store_keys nit: either '... we set --rowset_metadata_store_keys ...' or '... we set the --rowset_metadata_store_keys flag ...' http://gerrit.cloudera.org:8080/#/c/19650/5/src/kudu/tserver/tablet_server-test.cc@4685 PS5, Line 4685: encoded keys nit: the encoded keys -- To view, visit http://gerrit.cloudera.org:8080/19650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b7ed75bdf7a2ff0e16c2670f1a6f9819ee8e8d3 Gerrit-Change-Number: 19650 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 28 Mar 2023 04:03:42 +0000 Gerrit-HasComments: Yes
