[jira] [Commented] (HADOOP-12635) Adding Append API support for WASB

["Chris Nauroth (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-12635?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15090257#comment-15090257 ] 

Chris Nauroth commented on HADOOP-12635:
----------------------------------------

[~dchickabasapa], thank you for the patch.  Here are a few comments.

# Please let me know if I'm missing something, but it appears there is a fundamental problem in that {{AzureNativeFileSystemStore#retrieveAppendStream}} is not atomic.  First, it checks the metadata for a prior append lease, and if found, throws an exception.  This logic is not coordinated on a lease, so 2 concurrent processes could get past these checks for the same blob, and then call the {{BlockBlobAppendStream}} constructor.  Then, both processes would call {{BlockBlobAppendStream#updateBlobAppendMetadata}}.  One process would win the race for the blob lease and set the append lease in metadata.  The other process would block (not error) retrying blob lease acquisition.  (See {{SelfRenewingLease}} constructor.)  Eventually, it would acquire the blob lease, and set the append lease for itself in metadata.  At this point, there are 2 processes both owning a {{BlockBlobAppendStream}} pointing to the same blob.  I'm not sure what happens next.  Would both processes independently append and commit their own blocks?  Whatever happens, it's a violation of single-writer semantics.  In HDFS, this sequence is atomic, so it's guaranteed that one of those processes would have acquired the lease and the other would have experienced an exception.  Does the whole {{AzureNativeFileSystemStore#retrieveAppendStream}} method need to be guarded by the lease?
# Please do not start background threads or thread pools from within constructors.  This is a pitfall that can lead to tricky edge cases where the background thread sees the object in a partially constructed state.  The JVM can even reorder ops in funny ways, making the background thread see seemingly impossible state.  Instead, start threads from a separate {{initialize}} method that you can call right after the constructor.  This will guarantee that the object is in a consistent state before threads observe it.  I know {{SelfRenewingLease}} and other parts of the Hadoop code start threads from a constructor.  It's not a good thing.  :-)
# {{APPEND_LEASE_TIMEOUT}} is 30 seconds and {{LEASE_RENEWAL_PERIOD}} is also 30 seconds.  That's going to cut it pretty close.  If another process tries to append right at the 30-second mark, then the lease renewal might not get ahead of it.  FWIW, the HDFS client typically uses a 1-second renewal period and the soft expiration is 60 seconds, beyond which another client may claim the lease.
# Various metadata operations like {{setOwner}} don't coordinate on the append lease.  I think that means any such metadata operations running concurrently with append activity would risk overwriting {{append_lease_last_modified}} with an old value, so you could experience lost updates.  Maybe this is OK in practice if the renewal period is made more frequent as per above?
# The HDFS client previously started a separate renewal thread per lease, much like what {{BlockBlobAppendStream}} does here.  This eventually became a scalability bottleneck with too many threads in applications that open multiple files concurrently.  We evolved to a design of a single lease renewer thread capable of servicing all renewal activity.  Let's keep this in mind as a future enhancement if excessive threads starts to become a problem.
# In general, it's not necessary to call {{toString()}} explicitly for exception and log messages.  Particularly in cases like this:
{code}
    LOG.debug("Opening file: {} for append", f.toString());
{code}
If you just pass {{f}}, then the interface of SLF4J accepts the {{f}} instance and only calls {{toString()}} internally if debug level is enabled.  It probably doesn't matter much for {{Path#toString}}, but it can improve performance if the class has a more expensive {{toString()}} implementation.
# I see style issues like lines that are too long.  A pre-commit run would find all such problems.


> 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.001.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)