KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/21673 )
Change subject: [ranger] enhance the robustness of key retrieval ...................................................................... Patch Set 5: (15 comments) Thanks for your reviews. http://gerrit.cloudera.org:8080/#/c/21673/3/src/kudu/ranger-kms/ranger_kms_client.cc File src/kudu/ranger-kms/ranger_kms_client.cc: http://gerrit.cloudera.org:8080/#/c/21673/3/src/kudu/ranger-kms/ranger_kms_client.cc@36 PS3, Line 36: Apach > nit: Apache Ranger Done http://gerrit.cloudera.org:8080/#/c/21673/3/src/kudu/ranger-kms/ranger_kms_client.cc@41 PS3, Line 41: _clien > nit: period in seconds Done http://gerrit.cloudera.org:8080/#/c/21673/3/src/kudu/ranger-kms/ranger_kms_client.cc@113 PS3, Line 113: MonoDelta::FromSeconds(FLAGS_ranger_kms_client_generate_key_max_retry_time_s); > Print warning logs if failed. Done http://gerrit.cloudera.org:8080/#/c/21673/3/src/kudu/ranger-kms/ranger_kms_client.cc@117 PS3, Line 117: > There is a backoff utility in rpc module named RpcRetrier::ComputeBackoff() Done 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 greate > Why? It would be great to provide the reasoning behind in this comment. Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@40 PS4, Line 40: /github.com/apache/ranger/blob/4e365456f65 > Why is this name? Shouldn't it be something that signifies the amount of t Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@108 PS4, Line 108: } > nit: add 'const'? Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@109 PS4, Line 109: > ditto Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@114 PS4, Line 114: MonoTime now; > Consider rewriting this while() cycle using "do {...} while()" to make sure Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@119 PS4, Line 119: > nit: 'retrying' might be misleading if next cycle is already beyond the dea Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@129 PS4, Line 129: epFor > Why to change 'now' here? It's not used anymore in the code below. Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@129 PS4, Line 129: koff_ms) > nit: make this condition first in the "if()" clause to avoid extra calls an Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@130 PS4, Line 130: > nit: convert into static constexpr const char*? Done 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 info Done http://gerrit.cloudera.org:8080/#/c/21673/4/src/kudu/ranger-kms/ranger_kms_client.cc@130 PS4, Line 130: > This 'exit' part is confusing -- exiting from where? Done -- 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: 5 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: Wed, 21 Aug 2024 03:38:35 +0000 Gerrit-HasComments: Yes
