[
https://issues.apache.org/jira/browse/HDFS-9144?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15014569#comment-15014569
]
James Clampffer commented on HDFS-9144:
---------------------------------------
Some feedback, as well as some questions to make sure I understand everything
correctly:
-needs to be rebased to head now that dn retry is added
-The synchronous FileSystem::Open isn't declared pure virtual, it probably
should be.
-hdfsConnect looks like it can leak an IoService if FileSystem::New returns
null. Unlikely but possible as far as I can tell.
-Why is GetRandomClientName declared inline? Is this just to trick the compiler
into letting it stay in the header? Probably not a whole lot to gain by
inlining a non-leaf function that large; consider moving implementation to a
.cc file?
-DataNodeConnectionImpl could probably have it's constructor implementation in
a .cc file. I think you could set conn_ in the initializer list to get rid of
the conn_.reset call in the ctor.
-BlockReaderImpl has a shared_ptr<DataNodeConnection> member, what is this
being shared with? Is that just so it can pass a reference to itself to
continuations?
-DataTransferSaslStream::Handshake still uses a templated callback which I
think a lot of this patch is moving away from. Fixing up those little pieces
can be deferred to a later jira as it looks like nothing is calling it at the
moment.
-Have you been able to run anything against the C++ or C APIs, with and without
memcheck, to verify that the refactor didn't introduce any subtle bugs?
Overall I think it's a solid set of abstractions.
> Refactor libhdfs into stateful/ephemeral objects
> ------------------------------------------------
>
> Key: HDFS-9144
> URL: https://issues.apache.org/jira/browse/HDFS-9144
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Affects Versions: HDFS-8707
> Reporter: Bob Hansen
> Assignee: Bob Hansen
> Attachments: HDFS-9144.HDFS-8707.001.patch,
> HDFS-9144.HDFS-8707.002.patch, HDFS-9144.HDFS-8707.003.patch
>
>
> In discussion for other efforts, we decided that we should separate several
> concerns:
> * A posix-like FileSystem/FileHandle object (stream-based, positional reads)
> * An ephemeral ReadOperation object that holds the state for
> reads-in-progress, which consumes
> * An immutable FileInfo object which holds the block map and file size (and
> other metadata about the file that we assume will not change over the life of
> the file)
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
