Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24978 )
Change subject: KUDU-3811 [tools] add 'kudu cluster check_location_maintenance' ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/24978/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24978/2//COMMIT_MSG@23 PS2, Line 23: since an RF<3 table cannot : survive the loss of a single replica This is quite superficial. If all tablets of such a RF<3 table aren't in the affected location, this is not quite relevant. Do we really want to have this in addition to rule 3? http://gerrit.cloudera.org:8080/#/c/24978/2//COMMIT_MSG@49 PS2, Line 49: and it does not look at : master replica placement. You mean it doesn't take into account any pending replica replacement/movement that the system catalog runs automatically in the background? Perhaps, it's a good idea at least to indicate if there are tablet replicas with replace attributes -- it might be a sign of current auto-rebalancer or `kudu cluster rebalance` activity. Those honor replica placement policies, so they tend to improve the situation in this sense, in general. However, it might be a good idea at least to reflect on that and give a notice that the snapshot that was used to check for location black-out viability is a subject to imminent updates. http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/kudu-tool-test.cc@1437 PS2, Line 1437: check_location_maintenance It's OK for robots, but it is very long string for humans. Consider making it more concise. Also, the idea behind the CLI tool is to have it hierarchical, i.e. an option is to have 'kudu cluster location' entry, and then have sub-entries in there. http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/tool_action_cluster.cc@252 PS2, Line 252: cout << " [FAIL] " << low_rf_tables.size() : << " table(s) have replication_factor < 3:" << endl; : for (const auto& kv : low_rf_tables) { : cout << " " << kv.first << " (RF=" << kv.second << ")" << endl; : } : } Perhaps, we shouldn't fail the verification if there RF=1 tablets at locations that we aren't interested in putting into maintenance? Otherwise this tool cannot be used at any cluster that have a least one non-replicated Kudu table. http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/tool_action_cluster.cc@313 PS2, Line 313: locations.insert(ts.ts_location); > If we have any unlocated TServers, does this whole "safe to turn off region +1 Also, there might be a case when no locations are defined for a Kudu cluster at all. Probably, such a a special case should be taken care of in the very beginning of the tool, so the tool refuses to proceed. http://gerrit.cloudera.org:8080/#/c/24978/2/src/kudu/tools/tool_action_cluster.cc@334 PS2, Line 334: locations.find(FLAGS_location) == locations.end()) nit: please use map-util.h wrappers instead -- To view, visit http://gerrit.cloudera.org:8080/24978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd4cbaa8dab835d4d27270c06b4b0028aab8285b Gerrit-Change-Number: 24978 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Fri, 09 Oct 2026 22:25:29 +0000 Gerrit-HasComments: Yes
