Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20830 )
Change subject: Do not enter ALTERING state just for replication factor changed. ...................................................................... Patch Set 3: Code-Review+1 (5 comments) > (2 comments) > > > Patch Set 2: > > > > (3 comments) > > Thanks for the review. > I have added a test that fails without the fix. Actually without > the fix, it will time out while altering since the state of table > is altering forever. Thank you for adding a test for that! Overall the change looks to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG@17 PS2, Line 17: d, the former : condition is also needed. So > Sorry, I am not sure. The words here are trying to say that we can not make Sure: you are free to keep the wording as is, whatever serves your purpose best. I just thought that the point was to explain why this change wasn't breaking the functionality of changing the replication factor for tables that have tablets. But it seems you were trying to emphasize some other point :) http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2652 PS3, Line 2652: unique_ptr<KuduTableAlterer> table_alterer; : table_alterer.reset(client_->NewTableAlterer(kTableName)); nit: there is no need to reset the instance, it could be done in one-liner unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName)); http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2656 PS3, Line 2656: Does it make sense to add an extra check, making sure there are any tables in the table before trying to update its replication factor? It's possible to do so using the itest::GetTableLocations() utility function (you could check delete_tablet-itest.cc as a reference for itest::GetTableLocations() usage). http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2661 PS3, Line 2661: CHECK_OK Why not ASSERT_OK()? http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2667 PS3, Line 2667: CHECK_OK ditto: use ASSERT_OK instead of CHECK_OK? -- To view, visit http://gerrit.cloudera.org:8080/20830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 3 Gerrit-Owner: Song Jiacheng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng <[email protected]> Gerrit-Comment-Date: Tue, 02 Jan 2024 20:29:39 +0000 Gerrit-HasComments: Yes
