hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Loughran (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-14768) Honoring sticky bit during Deletion when authorization is enabled in WASB
Date Wed, 27 Sep 2017 13:08:00 GMT

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

Steve Loughran commented on HADOOP-14768:
-----------------------------------------

overall, code looks good. I'm relying on Thomas to review the internals of what's going on
& how it relates to Azure storage, so have worried more about integration with the rest
of the hadoop codebase, testing & maintenance

# A fair few of the changes I'm suggesting are actually related to the fact that this does
copy the existing code, so it's retaining those issues. Particular, what to do with exception
types to raise. I don't want different exceptions raised when auth is set vs unset, so its
probably safest to either leave them as is (raw IOEs, generally), or change both. Leaving
alone is the least risky; moving to tighter exceptions can be done as a separate task.

# One thing I want to see would be no duplication of the construction of error messages in
exceptions. This can be done with having constant strings for the text and using String.format.
Why? Guarantees the messages are consistent in codepaths, hence documentation, tests, etc.
(I don't care about log messages, except maybe the error logs, if we expect users to be looking
at them)

# Do the docs need to be updated? I don't see that they do, given
everything goes off the existing fs.azure.authorization option.

h3. NativeAzureFileSystem.java

I think maybe the entry point in delete() could be clearer, by having the normal delete in
a private method {{deleteWithoutAuth()}}. Then delete becomes

{code}
public boolean delete(Path f, boolean recursive,
    boolean skipParentFolderLastModifiedTimeUpdate) throws IOException {

  if (this.azureAuthorization) {
    return deleteWithAuthEnabled(f, recursive,
      skipParentFolderLastModifiedTimeUpdate);
  } else {
    return deleteWithoutAuth(....)
  }
{code}

And: have the two delete calls sequentially in the class, with {{deleteWithAuthEnabled}} immediately
after the {{deleteWithoutAuth}} method. Why? Lets someone reading down the file follow the
code easier: they'll see delete, see the simplest case, then see the auth case


* L1869, no need to call toString() on f, just go {{Log.debug("deleting file", f)}}, Saves
on the cost of the toString() call when not logging @ debug, handles null arguments. Same
for L1996, L2007, L2407, L2427, L2442, L2500...
* L1937, Maybe: throw {{org.apache.hadoop.fs.PathIsNotDirectoryException}} for consistency
with other calls & filesystems. (though only if "classic" delete does it)

* L1953: my IDE thinks that you can get here with {{parentPath==null}, so {{parentPath.getParent()}}
will fail. I think it's not noticed that root is never a file, so all files must have a parent
entry. If you add {{assert parentPath != null;}} that's you making that declaration and the
IDE becomes happy again.
* L1984, 1987: Lines too long. Why not import static {{NativeAzureFileSystemHelper.*}} and
so invoke those methods
without needing the classname. This gives shorter lines and less verbose code, here and on
lines 2030-2033

* L1972, why not construct the text with a String.format, skip the toString calls + key
* L2017: again, use String.format() to build the exception, remove the toString calls. Guarantees
that a null parentPath is handled.
* L2019, move to {{new ArrayList<>()}}
* L2050: Maybe: throw {{PathIsNotEmptyDirectoryException}} here
* L2076: log string doesn't include the {} for the value of {{f.toString}}. Add it, remove
the toString()
* L2088. Issue: if a recursive delete failed, should that update the last modified time of
the parent dir? Or is it as the actual dir being modified is itself not deleted, you don't
have to. If that's true, given the dir being deleted is still there, does its modtime need
to be changed?
* L2476: break the line before "throws"
* L2484: use Collection.addAll() to add list
* L2500: move to LOG.debug("...for {}", metadata.getKey())


h3. {{TestNativeAzureFileSystemAuthorization}}

If you import static ContractTestUtils.* you can clean up the code a lot, by not having to
reference it before every single assertPathExists/DoesNotExist, etc call.

1.to stop checkstyle complaining, make
{code}
protected final short stickybitPermissionConstant = 1700;
{code}

a static field with capital letters

{code}
protected static final short STICKYBIT_PERMISSION_CONSTANT = 1700;
{code}



2. A lot of the {{addAuthRuleForOwner}} lines are getting too long. Rather than splitting
them, why not
make the read and write enum string values constants in the class, and use everywhere:

{code}
protected static final String READ = WasbAuthorizationOperations.READ.toString();
protected static final String WRITE = WasbAuthorizationOperations.WRITE.toString();
{code}

* L607, L825, L916: just use {{assertFalse}}.
* L645, L727: use {{assertTrue}}
* {{testSingleFileDeleteWithStickyBitNegative}}. If delete() is expected to throw an exception,
then surely L687-689 won't execute. 

For operations which throw exceptions, we tend to use LambdaTestUtils.intercept(), which gives
us that exception, though I don't think anyone has mixed that with doAs yet. There's always
try/catch and GenericTestUtils.assertAssertionContains to catch and examine exceptions instead.

The checkstyle warnings need addressing. They're minor, but it's good to fight that losing
battle of "reduce checkstyle complaints"
{code}
./hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/MockWasbAuthorizerImpl.java:162:
 public AuthorizationComponent(String wasbAbsolutePath,:3: Redundant 'public' modifier. [RedundantModifier]
./hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java:52:
 protected final short stickybitPermissionConstant = 1700;:25: Variable 'stickybitPermissionConstant'
must be private and have accessor methods. [VisibilityModifier]
{code}

> Honoring sticky bit during Deletion when authorization is enabled in WASB
> -------------------------------------------------------------------------
>
>                 Key: HADOOP-14768
>                 URL: https://issues.apache.org/jira/browse/HADOOP-14768
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: fs/azure
>            Reporter: Varada Hemeswari
>            Assignee: Varada Hemeswari
>              Labels: fs, secure, wasb
>         Attachments: HADOOP-14768.001.patch, HADOOP-14768.002.patch, HADOOP-14768.003.patch,
HADOOP-14768.003.patch, HADOOP-14768.004.patch, HADOOP-14768.004.patch, HADOOP-14768.005.patch,
HADOOP-14768.006.patch
>
>
> When authorization is enabled in WASB filesystem, there is a need for stickybit in cases
where multiple users can create files under a shared directory. This additional check for
sticky bit is reqired since any user can delete another user's file because the parent has
WRITE permission for all users.
> The purpose of this jira is to implement sticky bit equivalent for 'delete' call when
authorization is enabled.
> Note : Sticky bit implementation for 'Rename' operation is not done as part of this JIRA



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message