HDFS-16963. Remove the unnecessary use of Optional from DistributedFileSystem #5505

szetszwo · 2023-03-23T09:16:14Z

Description of PR

In DfsPathCapabilities, the hasPathCapability(..) method may simply returns boolean.
In HdfsPathHandle, a constructor declares Optional parameters. It is a well known misuse of Optional.

See https://issues.apache.org/jira/browse/HDFS-16963

How was this patch tested?

Just a small code improvement. Tested by the existing tests and added a new test.

For code changes:

[Y] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
[NA] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
[NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
[NA] If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

…leSystem.

saxenapranav

There is a need to have three possibilities as discussed in comments. What if we create a new enum like Trilean (ex: https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/enums/Trilean.java). The Optional.empty() can be replaced with Trilean.UNKOWN.

Trilean trilean = DfsPathCapabilities.hasPathCapability(p, capability);
if(trilean != UNKNOWN) {return trilean.toBoolean();}

saxenapranav · 2023-03-23T11:34:11Z

...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java

-        capability);
-    if (cap.isPresent()) {
-      return cap.get();
+    if (DfsPathCapabilities.hasPathCapability(p, capability)) {


what if validatePathCapabilityArgs(path, capability) in DfsPathCapablity.hasPathCapablity gives CommonPathCapabilities.FS_SYMLINKS. Now, FileSystem.areSymlinksEnabled() can be true or false.
In earlier code, if FileSystem.areSymlinksEnabled() is false, we would retrn from old-line 2213 cap.get(). But now, it will go ahead and invoke super.hasPathCapability(p, capability);

@saxenapranav , thanks for reviewing this!

For FS_SYMLINKS, the result is the same since super.hasPathCapability(p, capability) always return the same value as FileSystem.areSymlinksEnabled(). However, it may be non-trivial.

Let's use upper case Boolean and use null to represent unknown.

Unfortunately, findbugs does not like Boolean. Let me revert the previous change and adds a test to ensure the correctness.

Hi. I understand that at present making call to FileSystem.hasPathCapability() is not going to make any issue as it checks for symlinks. But what if there is more logic added in the FileSystem.hasPathCapability() in future which expects the child classes to call the super method only in required cases.
My thought is that what ever is in FileSystem.hasPathCapablity should be abstract to any dev reading hasPathCapablity in DistributedFileSystem and WebHdfsFileSystem, and super.hasPathCapablity should be called only when required. if Optional<Boolean> cap gets true/false, it should return from there and not go to the super method. In my view, super method should only get invoked in case given capability doesn't match any of the

hadoop/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/DfsPathCapabilities.java

Lines 45 to 57 in b9fa5e0

case CommonPathCapabilities.FS_ACLS:

case CommonPathCapabilities.FS_APPEND:

case CommonPathCapabilities.FS_CHECKSUMS:

case CommonPathCapabilities.FS_CONCAT:

case CommonPathCapabilities.FS_LIST_CORRUPT_FILE_BLOCKS:

case CommonPathCapabilities.FS_MULTIPART_UPLOADER:

case CommonPathCapabilities.FS_PATHHANDLES:

case CommonPathCapabilities.FS_PERMISSIONS:

case CommonPathCapabilities.FS_SNAPSHOTS:

case CommonPathCapabilities.FS_STORAGEPOLICY:

case CommonPathCapabilities.FS_XATTRS:

return Optional.of(true);

case CommonPathCapabilities.FS_SYMLINKS:

.
I feel having an enum which can say TRUE, FALSE, UNKNOWN would be great. And the super method getting called only if DfsPathCapablities.hasPathCapablity gives UNKNOWN.

Thanks.

@saxenapranav , we cannot fix a potential "future" problem. Since there is a unit test, such future change will be detected by the test. At that time, we may think about how to solve the problem such as adding the enum.

I am completely understanding your point. But my only difference is that if something is returned from DfsPathCapabilities.hasPathCapability should be returned back to caller of `DistributedFileSystem.hasPathCapablity), without going to super's method.

I understand that super.hasPathCapability call doesn't pose any issue as of now and we can see for Enum in case more complications are added in future. I have a suggestion in the tests szetszwo@6ddc4f2. In this commit it has added asserts on the conditions used in FileSystem.hasPathCapablity Would be awesome if you can pick my commit in your pr.

Thanks.

@saxenapranav , I am fine to merge your test code. Thanks. Is there a way to do it in Github? Or I have to download it and merge it manually?

Thank you so much. I have opened pr on your fork: https://github.com/szetszwo/hadoop/pull/2/files. Thanks.

saxenapranav · 2023-03-23T11:35:17Z

...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java

-        capability);
-    if (cap.isPresent()) {
-      return cap.get();
+    if (DfsPathCapabilities.hasPathCapability(p, capability)) {


Same comment as given in WebHdfsFileSystem.

This reverts commit 5d1d04c.

szetszwo · 2023-03-24T04:07:13Z

The checkstyle warning is about the test method name testFS_SYMLINKS not matting the expected pattern. FS_SYMLINKS is for the constant name and this is just a test. Let's just ignore the checkstyle warning.

HDFS-16963: added asserts on the conditions used in FileSystem.hasPathCapablity

saxenapranav

Thanks for taking my suggestions. Just a comment for the addition of supportsSymlinks method call.

saxenapranav · 2023-03-24T08:46:14Z

...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java

-    if (cap.isPresent()) {
-      return cap.get();
+    if (DfsPathCapabilities.hasPathCapability(p, capability)
+        && supportsSymlinks()) {


why we are adding supportsSymlinks()? capablity can be anything different from FS_SYMLINKS

In szetszwo@6ddc4f2 , line 62 supportsSymlinks() returns false but line 64 dfs.hasPathCapability(..) still returns true.

Had added supportsSymlinks() returns false to just assert the behaviour at super.hasPathCapablity()

hadoop-yetus · 2023-06-09T07:57:31Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 59s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 1 new or modified test files.
			_ trunk Compile Tests _
-1 ❌	mvninstall	44m 3s	/branch-mvninstall-root.txt	root in trunk failed.
+1 💚	compile	1m 3s		trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1
+1 💚	compile	0m 58s		trunk passed with JDK Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	checkstyle	0m 41s		trunk passed
+1 💚	mvnsite	1m 1s		trunk passed
+1 💚	javadoc	0m 59s		trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1
+1 💚	javadoc	0m 39s		trunk passed with JDK Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	spotbugs	2m 50s		trunk passed
+1 💚	shadedclient	23m 55s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 51s		the patch passed
+1 💚	compile	0m 55s		the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1
+1 💚	javac	0m 55s		the patch passed
+1 💚	compile	0m 46s		the patch passed with JDK Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	javac	0m 46s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 21s		the patch passed
+1 💚	mvnsite	0m 50s		the patch passed
+1 💚	javadoc	0m 39s		the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1
+1 💚	javadoc	0m 36s		the patch passed with JDK Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	spotbugs	2m 46s		the patch passed
+1 💚	shadedclient	24m 50s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	2m 34s		hadoop-hdfs-client in the patch passed.
+1 💚	asflicense	0m 42s		The patch does not generate ASF License warnings.
		113m 48s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5505/1/artifact/out/Dockerfile
GITHUB PR	#5505
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 16a670a77711 4.15.0-206-generic #217-Ubuntu SMP Fri Feb 3 19:10:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `ad7ad57`
Default Java	Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-ga~~us1-0ubuntu1~~20.04-b09
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5505/1/testReport/
Max. process+thread count	706 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5505/1/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

HDFS-16963. Remove the unnecessary use of Optional from DistributedFi…

3698b88

…leSystem.

This comment was marked as outdated.

Sign in to view

saxenapranav suggested changes Mar 23, 2023

View reviewed changes

Use Boolean.

5d1d04c

This comment was marked as outdated.

Sign in to view

szetszwo added 2 commits March 24, 2023 09:19

Revert "Use Boolean."

dd67aa4

This reverts commit 5d1d04c.

Add TestDfsPathCapabilities.

51f7a87

This comment was marked as outdated.

Sign in to view

szetszwo requested a review from jojochuang March 24, 2023 04:04

saxenapranav and others added 3 commits March 23, 2023 22:33

test to include the conditions used in FileSystem.hasPathCapablity

6ddc4f2

Merge pull request #2 from saxenapranav/dfsHasCapablity-saxenapranav

0ec6f5c

HDFS-16963: added asserts on the conditions used in FileSystem.hasPathCapablity

Added webhdfs test.

ad7ad57

saxenapranav reviewed Mar 24, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDFS-16963. Remove the unnecessary use of Optional from DistributedFileSystem #5505

HDFS-16963. Remove the unnecessary use of Optional from DistributedFileSystem #5505

szetszwo commented Mar 23, 2023 •

edited

This comment was marked as outdated.

saxenapranav left a comment

saxenapranav Mar 23, 2023

szetszwo Mar 23, 2023

szetszwo Mar 24, 2023

saxenapranav Mar 24, 2023

szetszwo Mar 24, 2023

saxenapranav Mar 24, 2023

szetszwo Mar 24, 2023

saxenapranav Mar 24, 2023

saxenapranav Mar 23, 2023

This comment was marked as outdated.

This comment was marked as outdated.

szetszwo commented Mar 24, 2023

saxenapranav left a comment

saxenapranav Mar 24, 2023

szetszwo Mar 24, 2023

saxenapranav Mar 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

hadoop-yetus commented Jun 9, 2023

	case CommonPathCapabilities.FS_ACLS:
	case CommonPathCapabilities.FS_APPEND:
	case CommonPathCapabilities.FS_CHECKSUMS:
	case CommonPathCapabilities.FS_CONCAT:
	case CommonPathCapabilities.FS_LIST_CORRUPT_FILE_BLOCKS:
	case CommonPathCapabilities.FS_MULTIPART_UPLOADER:
	case CommonPathCapabilities.FS_PATHHANDLES:
	case CommonPathCapabilities.FS_PERMISSIONS:
	case CommonPathCapabilities.FS_SNAPSHOTS:
	case CommonPathCapabilities.FS_STORAGEPOLICY:
	case CommonPathCapabilities.FS_XATTRS:
	return Optional.of(true);
	case CommonPathCapabilities.FS_SYMLINKS:

HDFS-16963. Remove the unnecessary use of Optional from DistributedFileSystem #5505

Are you sure you want to change the base?

HDFS-16963. Remove the unnecessary use of Optional from DistributedFileSystem #5505

Conversation

szetszwo commented Mar 23, 2023 • edited

Description of PR

How was this patch tested?

For code changes:

This comment was marked as outdated.

saxenapranav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

szetszwo commented Mar 24, 2023

saxenapranav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

hadoop-yetus commented Jun 9, 2023

szetszwo commented Mar 23, 2023 •

edited