Yingchun Lai 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 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/19650/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19650/4//COMMIT_MSG@9 PS4, Line 9: We can cache the encoded min/max keys in rowset metadata to help : tablets to bootstrap without having to fully initializing the CFileReaders : for the key columns of each rowset. Is there any bug you encountered if not updating min/max key in metadata? http://gerrit.cloudera.org:8080/#/c/19650/2/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/19650/2/src/kudu/tablet/cfile_set.h@150 PS2, Line 150: // Use for unit test only. > I declare that friends hope that unit tests can use private functions and d The functions are more or less similar to public get()/set() member functions, they hide (only) the name of the private datum, but they don’t hide the existence of the private datum. If the functions are encapsulating complex logic, it's OK. See https://isocpp.org/wiki/faq/friends#friends-and-encap http://gerrit.cloudera.org:8080/#/c/19650/4/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/19650/4/src/kudu/tablet/cfile_set.cc@213 PS4, Line 213: if (FLAGS_rowset_metadata_store_keys) { The function name is 'LoadMinMaxKeys' which means only update member min/max_encoded_key_, but now take effect on 'rowset_metadata_'. How about move it out of this function and to the only caller in L173, if (FLAGS_rowset_metadata_store_keys && rowset_metadata_->has_encoded_keys()) { ... } else { ... } // Update the min/max key in metadata explicitly if user specified. if (FLAGS_rowset_metadata_store_keys && !rowset_metadata_->has_encoded_keys()) { rowset_metadata_->set_min_encoded_key(min_encoded_key_); rowset_metadata_->set_max_encoded_key(max_encoded_key_); } -- 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: 4 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: Mon, 27 Mar 2023 16:29:34 +0000 Gerrit-HasComments: Yes
