Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17674 )
Change subject: KUDU-3304: [Alter] Support to alter table's replication factor ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG@13 PS10, Line 13: add nit: adds http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG@13 PS10, Line 13: supported : in CLI tool Could you add a small blurb to explain that the CLI tool returns without waiting for the RF change to be effective in the sense having all replicas to be up and running with all the data fully replicated? http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/client/client-test.cc@8539 PS10, Line 8539: client_->data_->meta_cache_->ClearCache(); I'm curious how this works for meta cache of other clients which might be working with the table in parallel. Is that true that the meta cache of other clients working with the table will eventually be automatically updated once they detect a change in the version of the table's schema? Does it make sense to add a smoke test scenario like writing into the table while its replica factor is being changed? I guess changing the RF from 3 to 1 (or from 5 to 3) should be fine, but if changing the RF from 1 to 3 write operations could fail if the data cannot be re-replicated to a majority of new tablet replicas fast enough? http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/master/catalog_manager.cc@3062 PS4, Line 3062: "table name $0 is already reserved > Should this be also be set to true if table's replication factor is being u Ah, it seems this one is always a derivative of 'has_metadata_changes' and whether table isn't being dropped. http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/master/catalog_manager.cc@262 PS10, Line 262: count nit: factor http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc@988 PS10, Line 988: if (replication_factor <= 0) { : return Status::InvalidArgument(Substitute( : "Invalid replication factor: $0, it should be set higher than 0.", replication_factor)); : } It seems this check is redundant since the minimum value of the replication factor is being checked by catalog manager itself? http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc@994 PS10, Line 994: nit: an extra line? -- To view, visit http://gerrit.cloudera.org:8080/17674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aa2d5b12c508ba761fa9410ad1a465cf31fb7d7 Gerrit-Change-Number: 17674 Gerrit-PatchSet: 10 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 06 Aug 2021 07:39:39 +0000 Gerrit-HasComments: Yes
