[
https://issues.apache.org/jira/browse/HDFS-2928?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13208969#comment-13208969
]
Aaron T. Myers commented on HDFS-2928:
--------------------------------------
Patch is looking better, Uma. A few more comments:
# Let's add a default impl for createNNProxyWithNamenodeProtocol as well, so we
don't have to modify NameNodeConnector at all.
# I see that you addressed my comment about setting up retries in DFSUtil, but
then you made ClientNamenodeProtocolTranslatorPB call DFSUtil, which wasn't my
intention. I see why you did this (several other places call the
ClientNamenodeProtocolTranslatorPB constructor) but I actually think that
ideally the translators wouldn't be responsible for _setting up_ the underlying
proxy at all. i.e. the only translator ctor should be one which takes an
already-constructed proxy as a parameter. This is a separate refactor, though,
and so can be punted to another JIRA, if you want.
# Let's implement ClientNamenodeProtocolTranslatorPB(InetSocketAddress,
Configuration, UserGroupInformation) in terms of
ClientNamenodeProtocolTranslatorPB(ClientNamenodeProtocolPB), rather than
assigning rpcProxy in both constructors.
# Rather than return early in the case of "!withRetries" in both
createNNProxyWithNamenodeProtocol and createNNProxyWithClientProtocol, let's
just only set up the retryProxy in the case of "withRetries" being true. Seems
a little clearer to me.
# I see what you mean about the difficulty of testing this change, since it's
mostly just a refactor. The only test I can imagine which would test actual
functionality, rather than code structure, would be to somehow verify that the
create() call isn't retried without failing over in the case of
AlreadyBeingCreatedExceptions. But, that also seems difficult to write a test
for and not really worth it. The test as implemented seems fragile and is
really just asserting current code structure, so I think we should just rip it
out. Running the existing tests as you did should be sufficient to test this
change. Sorry I misdirected you there. Removing that also means we can scrap
the changes in NamenodeProtocolTranslatorPB.
> HA: ConfiguredFailoverProxyProvider should not create a NameNode proxy with
> an underlying retry proxy
> -----------------------------------------------------------------------------------------------------
>
> Key: HDFS-2928
> URL: https://issues.apache.org/jira/browse/HDFS-2928
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: ha, hdfs client
> Affects Versions: HA branch (HDFS-1623)
> Reporter: Aaron T. Myers
> Assignee: Uma Maheswara Rao G
> Priority: Minor
> Attachments: HDFS-2928.patch, HDFS-2928.patch
>
>
> This is to address the following TODO in ConfiguredFailoverProxyProvider:
> {quote}
> // TODO(HA): This will create a NN proxy with an underlying retry
> // proxy. We don't want this.
> {quote}
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
