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: (3 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 > Not sure if it's able to, I thought this is the least opinionated approach. It's up to you. Yes, I agree it can be added later, if needed. http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc@149 PS5, Line 149: if (FLAGS_ranger_kms_url.empty() || FLAGS_encryption_cluster_key_name.empty()) { This line seems to be too long http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: PS4: > Thought about that, but we don't have positive tests for curl-util at all r Yup, we do have some scenarios using the embedded webserver in webserver-test.cc. Adding those in a separate changelist looks good to me as well. It's up to you, I just thought that logically separated pieces with test coverage might be good to have. -- 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: Thu, 24 Nov 2022 19:30:15 +0000 Gerrit-HasComments: Yes
