[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20830 ) Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Reviewed-on: http://gerrit.cloudera.org:8080/20830 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 41 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 11 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
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 10: Code-Review+2
(1 comment)
Thank you for the fix!
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc@2642
PS9, Line 2642: TEST_F(ReplicatedAlterTableTest,
CheckTableStateAfterReplicatedAlter) {
> This probably needs to add some codes to catalog_manager.cc to enable or di
AFAIK, for the majority of tests in the Kudu codebase it's been enough to make
sure the test pass with the fix, and just manually checking that it was failing
before the fix. Adding variations on fix/no-fix is an overkill.
The idea is to make sure the fix works as intended and to catch future
regressions. That's already enough.
--
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: 10
Gerrit-Owner: Song Jiacheng
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Song Jiacheng
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 17:15:13 +
Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#10). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 41 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/10 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 10 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Song Jiacheng 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 9:
(2 comments)
> Patch Set 9:
>
> (2 comments)
Thanks for the review!
I'm not sure about these two points, please tell me if I am wrong.
http://gerrit.cloudera.org:8080/#/c/20830/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20830/9//COMMIT_MSG@10
PS9, Line 10: state
> Add: until the request timeout.
Actually this table will stay in ALTERING state forever, because it could not
get any tablet reports. The table could exit from ALTERING state only if it
receives some tablet reports.
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc@2642
PS9, Line 2642: TEST_F(ReplicatedAlterTableTest,
CheckTableStateAfterReplicatedAlter) {
> How about use TEST_P to test the case also: without the fix, alter replica
This probably needs to add some codes to catalog_manager.cc to enable or
disable the fix, doesn't it? If so, I think it might not worth that. Please
tell me if there is another way to disable the fix. Thanks!
--
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: 9
Gerrit-Owner: Song Jiacheng
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Song Jiacheng
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 09:47:23 +
Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Song Jiacheng 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 9: (3 comments) > Patch Set 6: > > (2 comments) Thanks for the review! 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 table_alterer(client_->NewTableAlterer(kTableName)); : ASSERT_OK(table_alterer->DropRangePartition(schema_.NewRow > nit: there is no need to reset the instance, it could be done in one-liner Done http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2661 PS3, Line 2661: kTable > Why not ASSERT_OK()? Done http://gerrit.cloudera.org:8080/#/c/20830/3/src/kudu/integration-tests/alter_table-test.cc@2667 PS3, Line 2667: > ditto: use ASSERT_OK instead of CHECK_OK? Done -- 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: 9 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 09:46:07 + Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Wang Xixu 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 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20830/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20830/9//COMMIT_MSG@10
PS9, Line 10: state
Add: until the request timeout.
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/20830/9/src/kudu/integration-tests/alter_table-test.cc@2642
PS9, Line 2642: TEST_F(ReplicatedAlterTableTest,
CheckTableStateAfterReplicatedAlter) {
How about use TEST_P to test the case also: without the fix, alter replica
factor will timeout?
--
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: 9
Gerrit-Owner: Song Jiacheng
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Song Jiacheng
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 09:19:17 +
Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#9). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 42 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/9 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 9 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#8). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 42 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/8 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 8 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#7). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 42 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/7 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 7 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
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 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/20830/6/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/20830/6/src/kudu/integration-tests/alter_table-test.cc@2661 PS6, Line 2661: cluster_->mini_tablet_server(i)->server()-> I think here we are interested what the system catalog (i.e. master) knows about the state of the table, not tablet servers. Why not to use itest::GetTableLocations() here? http://gerrit.cloudera.org:8080/#/c/20830/6/src/kudu/integration-tests/alter_table-test.cc@2665 PS6, Line 2665: style nit: wrong indent -- 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: 6 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Comment-Date: Wed, 03 Jan 2024 17:38:28 + Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#6). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 43 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/6 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 6 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#5). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 42 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/5 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 5 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#4). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 39 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/4 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 4 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
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 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 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Comment-Date: Tue, 02 Jan 2024 20:29:39 + Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Song Jiacheng 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: (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. 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: num > number Done http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG@17 PS2, Line 17: d, the former : condition is also needed. So > how about: the applicability of the changes for existing replicas is alread Sorry, I am not sure. The words here are trying to say that we can not make the table enter ALTERING state without checking the existing tablets number. -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Comment-Date: Thu, 28 Dec 2023 08:06:39 + Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#3). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the number of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc 2 files changed, 30 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/3 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 3 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
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 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG@9 PS2, Line 9: If we change the replication factor of a table without any tablets, : the table will stay in ALTERING state since exiting this state needs : tablet reports from the tablets of this table Do you mind adding a small test scenario for this to make sure the bug is fixed and to track future regressions? http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG@17 PS2, Line 17: num number http://gerrit.cloudera.org:8080/#/c/20830/2//COMMIT_MSG@17 PS2, Line 17: the check of : tablet number is also needed how about: the applicability of the changes for existing replicas is already accounted for by the 'table->num_tablets() > tablets_to_drop.size()' condition. -- 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: 2 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 23 Dec 2023 00:13:25 + Gerrit-HasComments: Yes
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20830 to look at the new patch set (#2). Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the num of replicas has been changed, the check of tablet number is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/master/catalog_manager.cc 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/2 -- 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: newpatchset Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 2 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Do not enter ALTERING state just for replication factor changed.
Song Jiacheng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20830 Change subject: Do not enter ALTERING state just for replication factor changed. .. Do not enter ALTERING state just for replication factor changed. If we change the replication factor of a table without any tablets, the table will stay in ALTERING state since exiting this state needs tablet reports from the tablets of this table. for now the condition of entering ALTERING state is as below: has_metadata_changes && (table->num_tablets() > tablets_to_drop.size() || num_replicas_changed); Actually even if the num of replicas has been changed, the former condition is also needed. So rollback the code to: has_metadata_changes && table->num_tablets() > tablets_to_drop.size(); Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 --- M src/kudu/master/catalog_manager.cc 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20830/1 -- 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: newchange Gerrit-Change-Id: I1d20da7c0dd5912790aaa46e9fff366b2973d7a4 Gerrit-Change-Number: 20830 Gerrit-PatchSet: 1 Gerrit-Owner: Song Jiacheng
