hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Loughran (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-15660) ABFS: Add support for OAuth
Date Mon, 13 Aug 2018 21:46:00 GMT

    [ 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 &lt;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: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message