[
https://issues.apache.org/jira/browse/HDFS-5673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13852409#comment-13852409
]
Chris Nauroth commented on HDFS-5673:
-------------------------------------
Thanks for the review, Haohui. I'm going to get to work on a v2 patch.
bq. Acl.Builder can initialize the entries field along with the declaration.
There are 2 constructors right now: one with specified capacity (used by
product code, always set to 32) and one without capacity (convenience for test
code). I'll change to a single constructor that always uses 32 capacity.
bq. The constructor of Acl does not need to copy the list.
If we don't copy the list, then the immutability guarantee is weaker. After
building, a caller could go back and add more entries through the builder,
which would sneak in and mutate the list. I'd prefer to keep the copy.
bq. It seems to me that you can pass in a wrapper for Acl to indicate that the
Acl list is well-formed.
I'll investigate this. If I understand correctly, we'd convert an untrusted
ACL spec to an instance of the well-formed object immediately, and that's what
we would pass around between the internal helper methods. The type signatures
would enforce that we're only passing around properly sorted lists, so the
precondition checks might not even be required at that point.
bq. I'm not sure that why AclTransformation needs to be a functor.
This was intended to avoid code duplication in all of the ACL modification
methods in {{FSNamesystem}}. In the current HDFS-5658 patch, we're starting to
see some of that code duplication related to holding the namesystem lock and
checking permissions. With a functor, we'd be able to have a single helper
method in {{FSNameystem}} implementing that workflow, and each ACL modification
method can pass in a different functor instance.
bq. I think the code is probably overly optimized... Obviously it is turning an
O ( n ) algorithm to an O(nlogn) algorithm, but n is fairly small...
I believe that's actually O(N^2), or perhaps more accurately O(N*M) if you want
to track size of the existing ACL and size of the ACL spec separately as N and
M respectively. Still, I understand your point. I had an earlier version of
the code that did multiple iterations. I found that it didn't really simplify
things though. It seems the complexity is driven not by the iteration, but
rather the rules for ACLs around automatic mask calculations, copying of some
of the default entries from the access entries if unspecified, and enforcing
the validation. I'll play around with this a little more though and see if I
can find a balance.
> Implement logic for modification of ACLs.
> -----------------------------------------
>
> Key: HDFS-5673
> URL: https://issues.apache.org/jira/browse/HDFS-5673
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Affects Versions: HDFS ACLs (HDFS-4685)
> Reporter: Chris Nauroth
> Assignee: Chris Nauroth
> Attachments: HDFS-5673.1.patch
>
>
> This patch will include the core logic for modification of ACLs. This
> includes support for all user-facing APIs that modify ACLs. This will cover
> access ACLs, default ACLs, automatic mask calculations, automatic inference
> of unprovided default ACL entries, and validation to prevent creation of an
> invalid ACL.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
