Mailing-List: contact common-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-issues@hadoop.apache.org
Date: Wed, 25 Feb 2015 01:28:05 +0000 (UTC)
From: "Andrew Wang (JIRA)" <jira@apache.org>
To: common-issues@hadoop.apache.org
Message-ID: <JIRA.12776624.1424507801000.6967.1424827685554@Atlassian.JIRA>
In-Reply-To: <JIRA.12776624.1424507801000@Atlassian.JIRA>
References: <JIRA.12776624.1424507801000@Atlassian.JIRA>
 <JIRA.12776624.1424507801635@arcas>
Subject: [jira] [Commented] (HADOOP-11620) Add Support for Load Balancing
 across a group of KMS servers for HA
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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)