[
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: [email protected]
For additional commands, e-mail: [email protected]
