Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/23844 )
Change subject: [tools] Add --preserve_table_ids flag to unsafe_rebuild ...................................................................... Patch Set 2: (8 comments) Thank you for working on this! Just a first pass. Planning to take a look again later. http://gerrit.cloudera.org:8080/#/c/23844/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23844/2//COMMIT_MSG@7 PS2, Line 7: Add --preserve_table_ids flag to unsafe_rebuild nit: https://kudu.apache.org/docs/command_line_tools_reference.html#master-unsafe_rebuild will also need to be modified after this gets merged. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/kudu-admin-test.cc@3865 PS2, Line 3865: TestRebuildMasterPreserveTableIds Do you think a couple more unit tests need to be added that manipulates --tserver_enforce_access_control to verify that scan request doesn't fail with table id mismatch error. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/kudu-admin-test.cc@3980 PS2, Line 3980: if (original_table_id != rebuilt_table_id) { : LOG(INFO) << "As expected, table_id changed from " << original_table_id : << " to " << rebuilt_table_id; : } Make it simpler by using ASSERT_NE or similar macros and throw error when condition is not met instead of the other way around. Similar to what is done on line no. 3916. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc@68 PS2, Line 68: If true, preserve the original table IDs from the tablet servers' metadata Move this above and make necessary alignments to the whole text, to maintain uniformity across the code base. Like the one above: default_schema_version, 0, "The table schema version assigned to tables if one " http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc@67 PS2, Line 67: DEFINE_bool(preserve_table_ids, false, : "If true, preserve the original table IDs from the tablet servers' metadata " : "instead of generating new ones. This is useful when you need to maintain " : "compatibility with external systems (e.g., Impala, HMS) that reference " : "tables by their original IDs."); Move this above default_schema_version. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc@261 PS2, Line 261: !replica.tablet_status().table_id().empty() Here and below (line 319-320): Doesn't has_table_id() take care of this? Is it possible to have such a scenario where table id is empty? If yes, maybe cover that in unit test as well. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc@268 PS2, Line 268: Cannot preserve table_id for table '$0': tablet server does not provide table_id. " : "This may happen with older Kudu versions. Generating new table_id '$1'." This would be a post facto situation where event of unsuccessful table id preservation has already happened. Rewording this a little bit - "tablet server cannot provide a valid table id" should be enough. Maybe add a comment on why such scenario would happen. http://gerrit.cloudera.org:8080/#/c/23844/2/src/kudu/tools/master_rebuilder.cc@315 PS2, Line 315: scoped_refptr<TableInfo> table = FindOrDie(tables_by_name_, table_name); Question: Does this have a TableInfo source different from replica.tablet_status().table_id()? In other words, what would cause organized metadata from tablet servers be different from what is stored in replica? -- To view, visit http://gerrit.cloudera.org:8080/23844 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ae4353564922312d646f0323271d804e32e3b0d Gerrit-Change-Number: 23844 Gerrit-PatchSet: 2 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 09 Jan 2026 11:54:17 +0000 Gerrit-HasComments: Yes
