[
https://issues.apache.org/jira/browse/HADOOP-15660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16578941#comment-16578941
]
Steve Loughran commented on HADOOP-15660:
-----------------------------------------
h3. AbfsConfiguration
general: do a static import of ConfigurationKeys.* and so avoid needing to
qualify all the other references.
L287: add the (invalid) auth type to the exception text to help diagnostics in
the field.
h3. ConfigurationKeys
It'd be nice for all the new constants to have a javadoc entry which includes
\{@value} at the end, e.g
{code}
/** Prefix for auth type properties: {@value}. */
public static final String FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME =
"fs.azure.account.auth.type.";
{code}
h3. AccessTokenProvider
you don't need <p> tags in the javadocs; blank lines are enough.
h3. AzureADAuthenticator
Should these utility methods all use {{Precondition.checkArgument}} to validate
their args before trying to talk to AD?
L119: you can just use {{new Hashtable<>()}}, and ideally use a HashMap for its
reduced synchronization overhead
L220: great to see the content-type being checked; that's something the adl:
connector doesn't do consistently
L229: add some whitespace between ";" and "https".
FWIW, I'm wondering whether it'd be better to use a hadoop config option for
proxies, having had to field system property related proxy config issues with
ADL. There's a lot of support in Hadoop for propagating config options around,
but little for system properties
h3. ClientCredsTokenProvider and UserPasswordTokenProvider
should there be checks for any of the args being non-empty as well as non-null?
h3. QueryParams
switch from Hashtable to HashMap unless this is somehow impossible.
This should have a unit test to verify it builds query strings properly
h3. AbfsRestOperation
L52: why the switch from a static {{LOG}} to a per-instance {{logger}} for
logging?
L173: you can remove the isDebugEnabled guard and just go
LOG.debug(""HttpRequest: {}", httpOperation) ; SL4J will do the toString() on
demand, handle null values.
h2. Tests
There's no new tests here. As well as a unit test to validate {{QueryParams}},
if there are any which could be used to verify that token provider construction
(& config option getPassword) calls were added, it'd stop regressions creepting
in.
> ABFS: Add support for OAuth
> ---------------------------
>
> Key: HADOOP-15660
> URL: https://issues.apache.org/jira/browse/HADOOP-15660
> Project: Hadoop Common
> Issue Type: Sub-task
> Reporter: Da Zhou
> Assignee: Da Zhou
> Priority: Major
> Attachments: HADOOP-15660-HADOOP-15407-001.patch
>
>
> - Add support for OAuth
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
