The patch aims to

1. associate StorageDirectory with StorageLocation, instead of java.io.File (replacing calls to StorageDirectory#getRoot by StorageDirectory#getStorageLocation)
2. remove references to StorageLocation#getFile

so that StorageLocation need not be associated with a java.io.File.

Updated patch to work with the new patches of HDFS-10636 and HDFS-10637.

Posting an updated patch to work with the most recent patch for HDFS-10637.

Hi, Virajith Jalaparti

   try {
   File file = new File(uri.toString());
  String absPath = file.getAbsolutePath();
  uri = new URI("file", uri.getAuthority(), absPath, uri.getQuery(), uri.getFragment());
} catch (URISyntaxException e) {
   e.printStackTrace();
}

It should not swallow the exception. It can throw IOE.

Modified patch based on the final patch for HDFS-10637.

-1 overall

This message was automatically generated.

The checkstyle warnings are due to moving (existing) code.

The failing test cases are not related to the patch, and seem spurious. Both run successfully, locally.

No new tests were added as the patch (1) mainly replaces usage of StorageLocation::getFile with StorageLocation::getURI, and StorageLocation, and (2) moves code (makeBlockPoolDataDir) to a new location. Existing tests (e.g., TestDataNodeHotSwapVolumes) already test these code paths.

Hi Lei (Eddy) Xu, I have fixed this in the new patch (004). Please take a look when you get a chance. Thanks!

Hi, Virajith Jalaparti

Thanks for addressing my comments.

    // for local replicas, the replica location is assumed to be a file.
    File diskFile = null;
    try {
      diskFile = new File(replicaLocation.getUri());
    } catch (NullPointerException | IllegalArgumentException e) {
      diskFile = null;
    }

• Catching NullPointerException here seems suspicious to me. Could you tell which one is potentially null? replicaLocation or getUrl() -> baseUrl?

• For StorageLocation#toString(), it might be good to just print URI itself instead of its path to be more verbose.

• public File getBpFile(String bpid, String currentStorageDir) }}, renamed to {{getBpDir(...)?

• Should we mark StorageLocation#getFile as deprecated?

One more question, as we are going to remove File from StorageLocation. Should we just remove StorageLocation#file and StorageLocation#getFile() once for all? I took a simple scan of the code base, getFile() is used 35 times, while 10 usages are in the scope of StorageLocation and StorageDirectory, while rest of them are all in the tests. So I think it'd be rather safe to remove them in hadoop 3. What do you think?

• Should StorageLocation#matchesStorageDirectory()}}s be moved to {{StorageDirectory class?

Hi Lei (Eddy) Xu, Thank you for the comments!

Should we mark StorageLocation#getFile as deprecated?

Should we just remove StorageLocation#file and StorageLocation#getFile() once for all?

So, are you proposing to replace all invocations of StorageLocation#getFile() with, say, StorageLocation#getURI()? I think this makes sense. The reason I didn't do this was because some of the tests use File#getCanonicalPath to compare File objects. I don't think an equivalent exists for URIs unless we explicitly add a StorageLocation#getCanonicalURI. However, as an equivalent call does not exist in java.net.URI, this would be a non-standard way of doing it.

One solution for this would be to modify the tests to replace calls to File#getCanonicalPath to use normalized URIs.

In StorageDirectory, we can replace StorageLocation#getFile() with StorageLocation#getURI(), and try to resolve it as a File internally.

Does this solution make sense?

Should StorageLocation#matchesStorageDirectory()}}s be moved to {{StorageDirectory class?

Any particular reason for this? If we move it to StorageDirectory, we would have a StorageDirectory#matchesStorageLocation function, which seems equivalent to what we have currently.

Catching NullPointerException here seems suspicious to me.

I added this to address the case where replicaLocation.getUri() might return null. However, if replicaLocation is constructed correctly, that should not be the case. I will remove this.

I agree with the others, and will address them in the next patch.

Attaching a new patch based on Lei (Eddy) Xu's comments. It completely removes File from StorageLocation, and uses an URI instead. One of the results of this is that VolumeFailureSummary.getFailedStorageLocations() now returns an array of strings that are URIs and not file paths. While VolumeFailureSummary is reported from the Datanode to the Namenode in the heartbeats, VolumeFailureSummary.getFailedStorageLocations() is not used to determine which StorageLocation s actually failed. If this changes in the future, it needs to be noted that these strings are actually URIs.

-1 overall

This message was automatically generated.

Failing test case is unrelated to the patch.

+1 . Thanks for the good work!

I re-run the failed test and it passes on my laptop. So I commit the patch to trunk.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10689 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10689/)
HDFS-10638. Modifications to remove the assumption that StorageLocation (lei: rev f209e93566b159c22054dcb276e45f23a2b7b7d1)

• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeVolumeFailureReporting.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeVolumeFailure.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDiskError.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataDirs.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/StorageLocation.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/LocalReplica.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDirectoryScanner.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestNameNodePrunesMissingStorages.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockScanner.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataStorage.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10769 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10769/)
HDFS-11098. Datanode in tests cannot start in Windows after HDFS-10638 (vinayakumarb: rev 69dd5fa2d43eefeec112f36b91a13513ac21a763)

• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/StorageLocation.java