Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13024 )
Change subject: [ksck] Make ksck tool to be more concurrency ...................................................................... Patch Set 1: (21 comments) http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@7 PS1, Line 7: to be more concurrency more concurrent http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@9 PS1, Line 9: fetch fetches http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@9 PS1, Line 9: when to : fetch when fetching http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@10 PS1, Line 10: which that http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@10 PS1, Line 10: much more : slower Remove http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@11 PS1, Line 11: CLI tool the CLI http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@11 PS1, Line 11: IDCs What's this stand for? I____ Data Center? http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@11 PS1, Line 11: clusters the cluster http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@12 PS1, Line 12: concurrency concurrent http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@12 PS1, Line 12: masters master http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@12 PS1, Line 12: tables table http://gerrit.cloudera.org:8080/#/c/13024/1//COMMIT_MSG@12 PS1, Line 12: to fetch when fetching http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.h@423 PS1, Line 423: of in http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.cc@60 PS1, Line 60: the same count with the " : "destination instances (masters, tservers or tables) will be created when " : "it's less than or equal to 0. I don't understand this part. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.cc@164 PS1, Line 164: CHECK_GT(num_masters, 0); I think returning a Status::NotFound is appropriate here, rather than a CHECK failure, unless you explain why num_masters > 0 is an invariant here. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck.cc@337 PS1, Line 337: servers_count At some point in the future this could be in the hundreds or thousands. I think we ought to set some reasonable default upper limit. The current number of 20 seems reasonable but if you wanted to bump it up to 30 or 40 I think that'd be fine. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc@505 PS1, Line 505: int num_tables = static_cast<int>(table_names.size()); : if (num_tables == 0) { : return Status::NotFound("No tables found"); : } Preserve the previous behavior of returning OK if there's no tables-- it's intended. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc@513 PS1, Line 513: num_tables This could be in the thousands. I think we need a reasonable default, like the 20 we had before for fetching from tablet servers. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc@542 PS1, Line 542: if (bad_tables.Load() > 0) { : return Status::NetworkError( : Substitute("failed to gather info from all tables: $0 of $1 had errors", : bad_tables.Load(), num_tables)); : } We need to provide at least one status from a failed OpenTable, too, so a user has some clue as to what went wrong opening tables. I think it'd be sufficient to log the status at error level. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc@552 PS1, Line 552: int num_tables = static_cast<int>(tables().size()); : if (num_tables == 0) { : return Status::NotFound("No tables found"); : } Same. This is an expected and non-error condition, so return OK. http://gerrit.cloudera.org:8080/#/c/13024/1/src/kudu/tools/ksck_remote.cc@560 PS1, Line 560: num_tables Again I think this will be too big for large clusters, creating possibly thousands of threads. -- To view, visit http://gerrit.cloudera.org:8080/13024 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f3f0e3f5115a46dd3afc83dda75e7241618eea4 Gerrit-Change-Number: 13024 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Tue, 16 Apr 2019 19:14:02 +0000 Gerrit-HasComments: Yes
