Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21673 )
Change subject: [ranger] enhance the robustness of key retrieval ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc File src/kudu/ranger-kms/ranger_kms_client.cc: http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@36 PS4, Line 36: set a value larger Why? It would be great to provide the reasoning behind in this comment. http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@40 PS4, Line 40: new_ranger_kms_user_take_effect_interval_s Why is this name? Shouldn't it be something that signifies the amount of time the KMS client retries? The rationale to set it higher than the described Ranger's parameter is a thing on its own -- it make sense to refer to it in the description, indeed. But there is no way to know its current setting for the KMS, right? http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@108 PS4, Line 108: MonoTime start nit: add 'const'? http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@109 PS4, Line 109: MonoTime deadline = start + ditto http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@114 PS4, Line 114: while ((now = MonoTime::Now()) < deadline) { Consider rewriting this while() cycle using "do {...} while()" to make sure that at least one attempt to fetch/construct the key is done regardless of the flag's setting. http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@119 PS4, Line 119: retrying: nit: 'retrying' might be misleading if next cycle is already beyond the deadline http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@129 PS4, Line 129: !s.ok() nit: make this condition first in the "if()" clause to avoid extra calls and operations in case of success Also, is it really necessary to do any timing comparison at all if !s.ok()? http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@129 PS4, Line 129: now = Why to change 'now' here? It's not used anymore in the code below. http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@130 PS4, Line 130: , exit This 'exit' part is confusing -- exiting from where? http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@130 PS4, Line 130: !!! nit: remove this hysteric part since it doesn't add anything useful or informative http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@130 PS4, Line 130: string timeout_msg nit: convert into static constexpr const char*? -- To view, visit http://gerrit.cloudera.org:8080/21673 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1fd3263ad6ba6d8e444036bb7d2158986098cb4b Gerrit-Change-Number: 21673 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 20 Aug 2024 01:34:06 +0000 Gerrit-HasComments: Yes
