KeeProMise commented on code in PR #8396:
URL: https://github.com/apache/hadoop/pull/8396#discussion_r3042792623
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/impl/MountTableStoreImpl.java:
##########
@@ -209,6 +214,56 @@ public RemoveMountTableEntryResponse removeMountTableEntry(
return response;
}
+ @Override
+ public RemoveMountTableEntriesResponse removeMountTableEntries(
+ RemoveMountTableEntriesRequest request) throws IOException {
+ List<String> failedPaths = new ArrayList<>();
+ List<MountTable> entriesToRemove = new ArrayList<>();
+ List<MountTable> allEntries =
getDriver().get(getRecordClass()).getRecords();
+ for (String path : request.getSrcPaths()) {
+ final MountTable partial = MountTable.newInstance();
+ partial.setSourcePath(path);
+ final Query<MountTable> query = new Query<>(partial);
+ List<MountTable> filtered = filterMultiple(query, allEntries);
+ MountTable deleteEntry = null;
+ if (filtered.size() == 1) {
+ deleteEntry = filtered.get(0);
+ }
+
+ if (deleteEntry != null) {
+ RouterPermissionChecker pc = RouterAdminServer.getPermissionChecker();
+ if (pc != null) {
+ try {
+ pc.checkPermission(deleteEntry, FsAction.WRITE);
+ entriesToRemove.add(deleteEntry);
+ } catch (IOException ioe) {
+ failedPaths.add(path);
+ }
+ }
+ } else {
+ failedPaths.add(path);
+ }
+ }
+
+ boolean anyRemoved = false;
+ Map<MountTable, Boolean> statuses =
getDriver().removeMultiple(entriesToRemove);
+ for (Map.Entry<MountTable, Boolean> mapEntry : statuses.entrySet()) {
+ if (!mapEntry.getValue()) {
+ failedPaths.add(mapEntry.getKey().getSourcePath());
+ } else {
+ anyRemoved = true;
+ }
+ }
Review Comment:
In MountTableStoreImpl.removeMountTableEntries, all failure cases are
collected into the same failedPaths list without distinguishing the reason:
// Case 1: entry not found
failedPaths.add(path);
// Case 2: permission denied (AccessControlException silently caught)
} catch (IOException ioe) {
failedPaths.add(path);
}
// Case 3: driver-level removal failure
failedPaths.add(mapEntry.getKey().getSourcePath());
As a result, the caller receives a flat failedEntriesKeys list with no way
to tell whether a given path failed due to it not existing, insufficient WRITE
permission, or a storage driver error.
This is also inconsistent with removeMountTableEntry (the single-entry
API), which throws AccessControlException directly on permission failure.
Was this intentional? For example, to avoid leaking path existence
information to unprivileged callers? If not, would it make sense to either
return separate lists per failure reason, or include a reason
code alongside each failed key?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
