Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19594 )
Change subject: [KUDU-3452] Make validate table creating task not affected ...................................................................... Patch Set 9: (14 comments) http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@9 PS9, Line 9: will be timeout times out http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@9 PS9, Line 9: Create Creating http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@9 PS9, Line 9: when one of three : tablet servers ? Looks like an unfinished sentence http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@10 PS9, Line 10: After that, create a two-replicas table also will : timeout even if there are enough tablet servers to place its : replicas. I'm not sure I understand where that two-replica table request appears from. Is it somehow related to the original request to create a table with three replicas? http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@15 PS9, Line 15: problem Why that's a problem? Was it a problem when the system didn't allow to create under-replicated tables? http://gerrit.cloudera.org:8080/#/c/19594/9//COMMIT_MSG@15 PS9, Line 15: fix fixes http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/master/catalog_manager.cc@5933 PS9, Line 5933: the tablet has finished selecting replicas the catalog manager has selected replicas for the tablet http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/master/catalog_manager.cc@5934 PS9, Line 5934: If a tablet selects replicas failed If selecting replicas for the tablet fails http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/master/catalog_manager.cc@5940 PS9, Line 5940: LOG(ERROR) << "Error selecting replicas for tablet " << (*it)->id() : << ", reason: " << s.ToString(); nit: please use Substitute() to format the error message -- it's much easier to read and understand what the output is going to be. I'd think of the following message here: Substitute("error selecting replicas for tablet $0: $1", (*it)->id(), s.ToString()) http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/master/catalog_manager.cc@5943 PS9, Line 5943: } else { : it++; : } readability nit: it's much easier to read if the condition with the shorter action/code should comes first if (s.ok()) { ++it; } else { ... } http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/tools/kudu-admin-test.cc@1913 PS9, Line 1913: TEST_F(AdminCliTest, TestNotAffectedCreatingTables) { I don't think kudu-admin-test.cc is a good place for such a test. There is nothing related to kudu admin tools here, that's just some test to test particular behavior aspect of the catalog manager. Please move this http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/tools/kudu-admin-test.cc@1938 PS9, Line 1938: heart beat heartbeat http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/tools/kudu-admin-test.cc@1952 PS9, Line 1952: will success should succeed http://gerrit.cloudera.org:8080/#/c/19594/9/src/kudu/tools/kudu-admin-test.cc@1964 PS9, Line 1964: will success should succeed -- To view, visit http://gerrit.cloudera.org:8080/19594 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I64668651d0e8f58b92cfb841bdb20617de6776f9 Gerrit-Change-Number: 19594 Gerrit-PatchSet: 9 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Mon, 24 Apr 2023 22:47:04 +0000 Gerrit-HasComments: Yes
