goiri commented on a change in pull request #3009:
URL: https://github.com/apache/hadoop/pull/3009#discussion_r637429648
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -337,6 +343,36 @@ public void refreshEntries(final Collection<MountTable>
entries) {
this.init = true;
}
+ /**
+ * Check if PATH is the trail associated with the Trash.
+ *
+ * @param path A path.
+ */
+ private static boolean isTrashPath(String path) throws IOException {
+ Pattern pattern = Pattern.compile(getTrashRoot() + TRASH_PATTERN + "/");
+ return pattern.matcher(path).find();
+ }
+
+ private static String getTrashRoot() throws IOException {
+ // Gets the Trash directory for the current user.
+ return FileSystem.USER_HOME_PREFIX + "/" +
+ RouterRpcServer.getRemoteUser().getUserName() + "/" +
+ FileSystem.TRASH_PREFIX;
+ }
+
+ /**
+ * Subtract a BaseTrash to get a new path.
+ *
+ * @param path Path to check/insert.
+ * @return New remote location.
+ * @throws IOException If it cannot find the location.
+ */
+ private static String subtractBaseTrashPath(String path)
Review comment:
Can we add a few unit tests for this method?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -381,18 +417,37 @@ public void clear() {
public PathLocation getDestinationForPath(final String path)
throws IOException {
verifyMountTable();
+ PathLocation res;
+ String tmpPath = path;
+ if (isTrashPath(tmpPath)) {
+ tmpPath = subtractBaseTrashPath(tmpPath);
+ }
+ String finalPath = tmpPath;
readLock.lock();
try {
if (this.locationCache == null) {
- return lookupLocation(path);
+ res = lookupLocation(finalPath);
+ } else {
+ Callable<? extends PathLocation> meh = new Callable<PathLocation>() {
Review comment:
It was there already but let's fix it :)
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -337,6 +343,36 @@ public void refreshEntries(final Collection<MountTable>
entries) {
this.init = true;
}
+ /**
+ * Check if PATH is the trail associated with the Trash.
+ *
+ * @param path A path.
+ */
+ private static boolean isTrashPath(String path) throws IOException {
Review comment:
Add a few tests.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -381,18 +417,37 @@ public void clear() {
public PathLocation getDestinationForPath(final String path)
throws IOException {
verifyMountTable();
+ PathLocation res;
+ String tmpPath = path;
+ if (isTrashPath(tmpPath)) {
+ tmpPath = subtractBaseTrashPath(tmpPath);
+ }
+ String finalPath = tmpPath;
readLock.lock();
try {
if (this.locationCache == null) {
- return lookupLocation(path);
+ res = lookupLocation(finalPath);
+ } else {
+ Callable<? extends PathLocation> meh = new Callable<PathLocation>() {
+ @Override
+ public PathLocation call() throws Exception {
+ return lookupLocation(finalPath);
+ }
+ };
+ res = this.locationCache.get(finalPath, meh);
}
- Callable<? extends PathLocation> meh = new Callable<PathLocation>() {
- @Override
- public PathLocation call() throws Exception {
- return lookupLocation(path);
+ if (isTrashPath(path)) {
+ List<RemoteLocation> remoteLocations = new ArrayList<>();
+ for (RemoteLocation remoteLocation : res.getDestinations()) {
+ remoteLocations.add(
+ new RemoteLocation(remoteLocation.getNsId(),
Review comment:
It might be better to create a new constructor where we pass a
RemoteLocation and a path?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -381,18 +417,37 @@ public void clear() {
public PathLocation getDestinationForPath(final String path)
throws IOException {
verifyMountTable();
+ PathLocation res;
+ String tmpPath = path;
+ if (isTrashPath(tmpPath)) {
+ tmpPath = subtractBaseTrashPath(tmpPath);
+ }
+ String finalPath = tmpPath;
readLock.lock();
try {
if (this.locationCache == null) {
- return lookupLocation(path);
+ res = lookupLocation(finalPath);
+ } else {
+ Callable<? extends PathLocation> meh = new Callable<PathLocation>() {
Review comment:
We should have something better than meh, and this is an ideal candidate
for using a lambda.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java
##########
@@ -381,18 +417,37 @@ public void clear() {
public PathLocation getDestinationForPath(final String path)
throws IOException {
verifyMountTable();
+ PathLocation res;
+ String tmpPath = path;
Review comment:
This whole pass to finalPath could be a method and the names could be a
little more self-explanatory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
