[
https://issues.apache.org/jira/browse/HDFS-9144?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15022209#comment-15022209
]
Bob Hansen commented on HDFS-9144:
----------------------------------
[~James Clampffer]: thanks again for taking the time to review this large patch.
{quote}
-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.
{quote}
The excessive inlining in the headers were mostly an artifact of refactoring;
that's how they were in the original. I've moved them all into .cc files.
bq. -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?
It's to ensure that the BlockReaderImpl's lifecycle is tied to the request, and
won't be freed until the final callback is complete in the read_handler. I
suppose it could be a unique_ptr, but I went with shared_ptr for consistency.
bq. -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.
I did as little in the SaslStream as possible, given its uncertain future. We
can revisit that when we revisit SaslStream.
bq. -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?
I did for patch 2; I'll do some more and report if anything comes up.
> 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,
> HDFS-9144.HDFS-8707.004.patch, HDFS-9144.HDFS-8707.005.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)
