[
https://issues.apache.org/jira/browse/HADOOP-12635?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15102790#comment-15102790
]
Chris Nauroth commented on HADOOP-12635:
----------------------------------------
Thank you for the new revision, [~dchickabasapa].
I had previously asked for {{ioThreadPool}} to be marked {{final}}, but I see
now that it isn't possible.
I have a few more items remaining. Much of this is nit-picky at this point.
We're getting close.
Mark {{BlockBlobAppendStream#LOG}} and {{NativeAzureFileSystemHelper#LOG}} as
{{private}}.
The comment's indentation here has an extra space:
{code}
try {
//Set the append lease if the value of the append lease is false
if (!updateBlobAppendMetadata(true, false)) {
{code}
There is a typo on "SelfRenewingLease" in this comment:
{code}
// Swallowing exception here as the lease is cleaned up by the
SeleRenewingLease object.
{code}
Please add the {{@Override}} annotation to {{UploaderThreadFactory#newThread}}.
The JavaDocs for the new overload of {{StorageInterface#uploadMetadata}} don't
mention the additional parameters. Since everything else in
{{StorageInterface}} has thorough JavaDocs, would you please correct that and
also add JavaDocs for all of the new methods? That's {{downloadBlockList}},
{{uploadBlock}} and {{commitBlockList}}.
Could you please move {{TestNativeAzureFileSystemAppend#setUp}} to the top of
the class? It's customary to put the {{@Before}} method at the top, so that's
where people will expect to find it.
{{TestNativeAzureFileSystemAppend#createBaseFileWithData}} is at risk of
leaking an open file if the {{write}} call throws an exception before the
chance to call {{close}}. Please use either try-finally or try-with-resources
to guarantee the {{close}} call happens. I also see the same problem in a few
of the {{@Test}} methods, so please fix it throughout the whole class.
I recommend using {{Configuration#setBoolean}} here ,so that you won't need to
take care of the string conversion:
{code}
conf.set(NativeAzureFileSystem.APPEND_SUPPORT_ENABLE_PROPERTY_NAME,
Boolean.toString(true));
{code}
In {{TestNativeAzureFileSystemAppend#testSingleAppenderScenario}}, please add a
call to {{GenericTestUtils.assertExceptionContains("Unable to set Append lease
on the Blob.", ioe);}}. This will guarantee that the {{IOException}} was
thrown for the expected reason, and it's not some other unrelated I/O error.
Please add a test that sets the configuration property to false and then
asserts that calling {{append}} results in an exception.
Please update src/site/markdown/index.md. We'll want to mention that WASB has
optional support for append by setting the configuration property. We'll also
want to call out in scary bold letters that it differs from HDFS append
semantics. The application has responsibility to enforce a single writer
externally, and failure to do so will result in unexpected behavior.
> Adding Append API support for WASB
> ----------------------------------
>
> Key: HADOOP-12635
> URL: https://issues.apache.org/jira/browse/HADOOP-12635
> Project: Hadoop Common
> Issue Type: Improvement
> Components: azure
> Affects Versions: 2.7.1
> Reporter: Dushyanth
> Assignee: Dushyanth
> Attachments: Append API.docx, HADOOP-12635-004.patch,
> HADOOP-12635.001.patch, HADOOP-12635.002.patch, HADOOP-12635.003.patch,
> HADOOP-12635.005.patch, HADOOP-12635.006.patch
>
>
> Currently the WASB implementation of the HDFS interface does not support
> Append API. This JIRA is added to design and implement the Append API support
> to WASB. The intended support for Append would only support a single writer.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
