[
https://issues.apache.org/jira/browse/HADOOP-11620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14335806#comment-14335806
]
Andrew Wang commented on HADOOP-11620:
--------------------------------------
Hi Arun, thanks for working on this. Broad strokes look good, just mostly nitty
stuff:
KMSClientProvider:
* Unused import
* Typo: presesnt
* Could we split the new parsing into functions?
* What is the purpose of the new constructor that takes a Path, and marking the
current one as @VisibleForTesting?
TestKeyProviderFactory:
* Unused new imports
TestKMS:
* Unused import, although unrelated to this patch
* Is the createProvider refactor related to this patch? Doesn't seem necessary.
LoadBalancingKMSClientProvider:
* getCurrentIdx is unused
* Let's use {{Time.monotonicNow}} rather than {{System.currentTimeMillis}},
better precision.
* {{seed}} is not quite the right term in the test constructor, maybe just
{{currentIdx}}?
* in doOp, let's use slf4j substitution instead of string concatenation for the
logs.
* In doOp, I'd recommend always printing the LOG warn on an exception (the else
case), then additionally log "Failed to contact any of the KMS in the load
balancer group, aborting." if we're at the end. It'd also be good to include
the IOE's message in the first log.
* doOp would also be more clear as a for loop, rather than nesting all these
doOp calls. Seems like recursion will lead to funky stacktraces too.
* getStartIdx, maybe rename to {{nextIdx}} as it's not strictly a getter? Since
this pre-increments, it makes using the test constructor a little more
difficult, would be easier if it did post-increment.
* Just using {{volatile}} isn't enough to safely increment {{currentIdx}}. I
guess it's not a big deal, but it'd be safer to use an AtomicInt here.
* Some lihes longer than 80 chars.
TestLoadBalancingKMSClientProvider:
* Needs an auto-formatter pass, the multi-line chained mocking should be double
indented
* Let's use some more descriptive messages than "Should fail" :)
> Add Support for Load Balancing across a group of KMS servers for HA
> -------------------------------------------------------------------
>
> Key: HADOOP-11620
> URL: https://issues.apache.org/jira/browse/HADOOP-11620
> Project: Hadoop Common
> Issue Type: Improvement
> Components: kms
> Affects Versions: 2.6.0
> Reporter: Arun Suresh
> Assignee: Arun Suresh
> Attachments: HADOOP-11620.1.patch, HADOOP-11620.2.patch,
> HADOOP-11620.3.patch, HADOOP-11620.4.patch
>
>
> This patch needs to add support for :
> * specification of multiple hostnames in the kms key provider uri
> * KMS client to load balance requests across the hosts specified in the kms
> keyprovider uri.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
