hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mike Yoder (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-10736) Add key attributes to the key shell
Date Fri, 27 Jun 2014 18:54:24 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-10736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14046274#comment-14046274
] 

Mike Yoder commented on HADOOP-10736:
-------------------------------------

{quote}
KeyProvider.java:
Use final on StringBuilder decl.
s/append( MessageFormat/append(MessageFormat/
s/) )/))/
s/)-1)/) - 1)/
If I'm reading toString() correctly, it looks like it will be [foo=bar],[baz=quux],[k=v].
IWBNI there was a space after the , so ], [.
{quote}

Done for all.  But oh man old coding conventions die hard.  I'm used to putting spaces around
my )'s!

{quote}
KeyShell.java:
init(): Map<> attributes could use a final decl. Ditto attrval, attr, and val.
s/( /(/ and s/ )/)/
{quote}

Done.

{quote}
If I'm reading the code correctly, I think the usage message should be:
"\nAttributes must be in attributes=value form\n", since I don't think you can really have
'attr = value' in the shell unless you are expecting three different args, right? You have
this correct further down in the USAGE string. Nit: I might change the message to be: "\nAttributes
and values must be specified as attr=val\n" (if you make this change, then also update the
USAGE and DESC strings below).
{quote}
Yeah, this is one of those shell things that the user would have to know about... "attribute
= value" is fine since it's one argument (and I trim() the strings), but attribute = value
is not since it's three.  How about...

{noformat}
Attributes must be in attribute=value form, or quoted
like "attribute = value"
{noformat}

{quote}
TestKeyShell.java:
In the comment, should jceks provider be JCEKS provider? Yeah, I know that's a real nit...
final on tmpDir decl. Ditto delArgs, listArgs, listArgsM, etc.
s/( /(/ and s/ )/)/
I guess while you're in the neighborhood, you might as well convert the import static org.junit.Assert.*
to a set of explicit import statics.
{quote}
Done for all.
{quote}
Consider using GenericTestUtils for matching the output.
{quote}
Hmmm.  Had a look, thanks for the pointer.  You're thinking of assertMatches()?  Maybe, but
a regex match might be overkill.  I'd have to escape my []'s.  I think I'll pass on this one.

Thanks!  New patch coming soon...

> Add key attributes to the key shell
> -----------------------------------
>
>                 Key: HADOOP-10736
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10736
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: security
>    Affects Versions: 3.0.0
>            Reporter: Mike Yoder
>            Assignee: Mike Yoder
>             Fix For: 3.0.0
>
>         Attachments: HADOOP-10736.patch
>
>
> The recent work in HADOOP-10696 added attribute-value pairs to keys in the KMS.  Now
the key shell needs to be updated to set/get these attributes.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Mime
View raw message