Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )
Change subject: KUDU-3423 Add support for Ranger KMS HA ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG@16 PS4, Line 16: If a server : responds with an error Is Ranges KMS able to respond with "503 Service Unavailable" or something like that? If so, maybe at least treat 5xx server errors like a non-availability event and switch to the next in the list? http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@141 PS4, Line 141: " " nit: could drop the space and join two parts of the string? http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@149 PS4, Line 149: FLAGS_ranger_kms_url.empty() Since now a list of comma-separated strings can be provided as value for --ranger_kms_url, maybe check that the result of parsing the contents gives non-empty set of strings? My concern is that the newly introduced libcurl functions return Status::OK() for empty set of URLs, and a misconfiguration for --ranger_kms_url might be detected only at later stage http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/ranger-kms/ranger_kms_client.cc File src/kudu/ranger-kms/ranger_kms_client.cc: http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/ranger-kms/ranger_kms_client.cc@63 PS4, Line 63: for (int i = 0; i < kms_urls_.size(); i++) { nit for here and below: since having the index isn't crucial, there could be more concise version for (const auto& url : kms_urls_) { ... } http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: PS4: Consider separating this newly added {fetch,post}-with-failover functionality into its own patch and adding test coverage as well. http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@61 PS4, Line 61: // one in the list. Maybe, also mention that it stops at the first successful fetch attempt and return the result. http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@78 PS4, Line 78: Status PostToURL(const std::vector<std::string>& urls, ditto: maybe mention what happens on the fist successful post http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.cc@135 PS4, Line 135: for (int i = 0; i < urls.size(); ++i) nit: since having the index isn't crucial here, consider more concise form of this for (const auto& url : urls) { ... } -- To view, visit http://gerrit.cloudera.org:8080/19271 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8 Gerrit-Change-Number: 19271 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 23 Nov 2022 22:06:25 +0000 Gerrit-HasComments: Yes
