From eafed67e03480b0ba36ac500bec22f46b0f75b1d Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Wed, 15 Jul 2026 18:23:39 -0700 Subject: [PATCH] HDDS-15877. Extend getFileStatus head-op (type-only) optimization to o3fs and remaining type-only callers Follow-up of HDDS-15678, which added a headOp (metadata/type-only) flag so ofs isDirectory/isFile skip the SCM pipeline refresh and block-location retrieval. - o3fs BasicOzoneFileSystem.isDirectory/isFile now use the head-op path instead of the deprecated super.isDirectory/isFile (which triggered a full getFileStatus + pipeline refresh); BasicOzoneClientAdapterImpl forwards headOp to OzoneBucket.getFileStatus. - DeleteKeyHandler type checks pass headOp=true. - Trash-root type checks fetch the status once instead of exists() + two getFileStatus() calls for the same path. Adds unit tests for headOp propagation through the o3fs adapter and for the o3fs FileSystem isDirectory/isFile behaviour. Co-authored-by: Cursor Change-Id: I2cf245c4731e662f0929ce30f731ce518b422813 --- .../ozone/shell/keys/DeleteKeyHandler.java | 4 +- .../fs/ozone/BasicOzoneClientAdapterImpl.java | 9 +- .../hadoop/fs/ozone/BasicOzoneFileSystem.java | 39 ++++- .../BasicRootedOzoneClientAdapterImpl.java | 12 +- .../TestBasicOzoneClientAdapterHeadOp.java | 136 ++++++++++++++++ .../fs/ozone/TestOzoneFileSystemHeadOp.java | 149 ++++++++++++++++++ 6 files changed, 336 insertions(+), 13 deletions(-) create mode 100644 hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicOzoneClientAdapterHeadOp.java create mode 100644 hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemHeadOp.java diff --git a/hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/keys/DeleteKeyHandler.java b/hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/keys/DeleteKeyHandler.java index d203e3d2fb73..667bde481d51 100644 --- a/hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/keys/DeleteKeyHandler.java +++ b/hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/keys/DeleteKeyHandler.java @@ -100,7 +100,7 @@ private void deleteFSOKey(OzoneBucket bucket, String keyName) return; } - if (bucket.getFileStatus(keyName).isDirectory()) { + if (bucket.getFileStatus(keyName, true).isDirectory()) { List ozoneFileStatusList = bucket.listStatus(keyName, false, "", 1); if (ozoneFileStatusList != null && !ozoneFileStatusList.isEmpty()) { @@ -122,7 +122,7 @@ private void deleteFSOKey(OzoneBucket bucket, String keyName) String toKeyName = new Path(userTrashCurrent, keyName).toUri().getPath(); if (isKeyExist(bucket, toKeyName)) { - if (bucket.getFileStatus(toKeyName).isDirectory()) { + if (bucket.getFileStatus(toKeyName, true).isDirectory()) { // if directory already exist in trash, just delete the directory bucket.deleteKey(keyName); return; diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java index d9033054ead8..39cce33637f1 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java @@ -414,9 +414,16 @@ public boolean deleteObjects(List keyNameList) { public FileStatusAdapter getFileStatus(String key, URI uri, Path qualifiedPath, String userName) throws IOException { + return getFileStatus(key, uri, qualifiedPath, userName, false); + } + + @Override + public FileStatusAdapter getFileStatus(String key, URI uri, + Path qualifiedPath, String userName, boolean headOp) + throws IOException { try { incrementCounter(Statistic.OBJECTS_QUERY, 1); - OzoneFileStatus status = bucket.getFileStatus(key); + OzoneFileStatus status = bucket.getFileStatus(key, headOp); return toFileStatusAdapter(status, userName, uri, qualifiedPath); } catch (OMException e) { diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java index 4f0cc8b60bd8..7ade8339232c 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java @@ -785,8 +785,15 @@ public Collection getTrashRoots(boolean allUsers) { try { if (!allUsers) { Path userTrash = new Path(trashRoot, userName); - if (exists(userTrash) && getFileStatus(userTrash).isDirectory()) { - ret.add(getFileStatus(userTrash)); + // headOp: only the entry type is needed here. Fetch once instead of + // calling exists()/getFileStatus() repeatedly for the same path. + try { + FileStatus userTrashStatus = getFileStatus(userTrash, true); + if (userTrashStatus.isDirectory()) { + ret.add(userTrashStatus); + } + } catch (FileNotFoundException ignored) { + // No trash root for this user. } } else { if (exists(trashRoot)) { @@ -834,6 +841,16 @@ public long getDefaultBlockSize() { @Override public FileStatus getFileStatus(Path f) throws IOException { + return getFileStatus(f, false); + } + + /** + * @param headOp when true, requests a metadata-only (type) check so the OM + * skips the pipeline refresh (SCM round-trip) and datanode + * sorting. Used by {@link #isDirectory(Path)}/{@link #isFile(Path)}, + * which only need the entry type. + */ + private FileStatus getFileStatus(Path f, boolean headOp) throws IOException { incrementCounter(Statistic.INVOCATION_GET_FILE_STATUS, 1); statistics.incrementReadOps(1); LOG.trace("getFileStatus() path:{}", f); @@ -842,7 +859,7 @@ public FileStatus getFileStatus(Path f) throws IOException { FileStatus fileStatus = null; try { fileStatus = convertFileStatus( - adapter.getFileStatus(key, uri, qualifiedPath, getUsername())); + adapter.getFileStatus(key, uri, qualifiedPath, getUsername(), headOp)); } catch (IOException ex) { if (ex instanceof OMException) { if (((OMException) ex).getResult() @@ -1014,17 +1031,25 @@ public FileStatus[] globStatus(Path pathPattern, PathFilter filter) } @Override - @SuppressWarnings("deprecation") public boolean isDirectory(Path f) throws IOException { incrementCounter(Statistic.INVOCATION_IS_DIRECTORY); - return super.isDirectory(f); + try { + // headOp: only the entry type is needed, so skip the pipeline refresh. + return getFileStatus(f, true).isDirectory(); + } catch (FileNotFoundException e) { + return false; + } } @Override - @SuppressWarnings("deprecation") public boolean isFile(Path f) throws IOException { incrementCounter(Statistic.INVOCATION_IS_FILE); - return super.isFile(f); + try { + // headOp: only the entry type is needed, so skip the pipeline refresh. + return getFileStatus(f, true).isFile(); + } catch (FileNotFoundException e) { + return false; + } } @Override diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java index 50842949af2c..261f3a83761b 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java @@ -760,9 +760,15 @@ public Collection getTrashRoots(boolean allUsers, } } else { Path userTrash = new Path(trashRoot, username); - if (fs.exists(userTrash) && - fs.getFileStatus(userTrash).isDirectory()) { - ret.add(fs.getFileStatus(userTrash)); + // Fetch the status once instead of exists() + two getFileStatus() + // calls for the same path. + try { + FileStatus userTrashStatus = fs.getFileStatus(userTrash); + if (userTrashStatus.isDirectory()) { + ret.add(userTrashStatus); + } + } catch (FileNotFoundException ignored) { + // No trash root for this user. } } } diff --git a/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicOzoneClientAdapterHeadOp.java b/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicOzoneClientAdapterHeadOp.java new file mode 100644 index 000000000000..71a99524faef --- /dev/null +++ b/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicOzoneClientAdapterHeadOp.java @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.ozone; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.lang.reflect.Field; +import java.net.URI; +import java.util.Collections; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdds.client.RatisReplicationConfig; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; +import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +/** + * Unit tests for headOp propagation through + * {@link BasicOzoneClientAdapterImpl#getFileStatus} (HDDS-15877). Uses a partial + * mock so no OM connection is required. + */ +public class TestBasicOzoneClientAdapterHeadOp { + + private static final URI URI_O3FS = URI.create("o3fs://bucket.vol/"); + private static final Path WORKING_DIR = new Path("/"); + + private BasicOzoneClientAdapterImpl adapter; + private OzoneBucket bucket; + + @BeforeEach + public void setUp() throws Exception { + adapter = mock(BasicOzoneClientAdapterImpl.class, CALLS_REAL_METHODS); + bucket = mock(OzoneBucket.class); + // Inject the mock bucket so getFileStatus can run without a live OM. + Field field = + BasicOzoneClientAdapterImpl.class.getDeclaredField("bucket"); + field.setAccessible(true); + field.set(adapter, bucket); + } + + @AfterEach + public void tearDown() { + // Ensure no Mockito stubbing state leaks into other test classes running in + // the same JVM. + Mockito.validateMockitoUsage(); + } + + private static OzoneFileStatus fileStatus(boolean isDir) { + OmKeyInfo keyInfo = new OmKeyInfo.Builder() + .setVolumeName("vol") + .setBucketName("bucket") + .setKeyName("key") + .setReplicationConfig(RatisReplicationConfig.getInstance( + HddsProtos.ReplicationFactor.THREE)) + .setOmKeyLocationInfos(Collections.emptyList()) + .setDataSize(0) + .setCreationTime(0) + .setModificationTime(0) + .setAcls(Collections.emptyList()) + .build(); + return new OzoneFileStatus(keyInfo, 512, isDir); + } + + @Test + public void headOpOverloadThreadsHeadOp() throws IOException { + when(bucket.getFileStatus(anyString(), anyBoolean())) + .thenReturn(fileStatus(false)); + + assertFalse(adapter.getFileStatus("key", URI_O3FS, WORKING_DIR, "user", true) + .isDir()); + + ArgumentCaptor headOp = ArgumentCaptor.forClass(Boolean.class); + verify(bucket).getFileStatus(anyString(), headOp.capture()); + assertTrue(headOp.getValue()); + } + + @Test + public void fourArgOverloadDoesNotUseHeadOp() throws IOException { + when(bucket.getFileStatus(anyString(), anyBoolean())) + .thenReturn(fileStatus(true)); + + assertTrue(adapter.getFileStatus("key", URI_O3FS, WORKING_DIR, "user") + .isDir()); + verify(bucket).getFileStatus(anyString(), eq(false)); + } + + @Test + public void fileNotFoundMappedToFileNotFoundException() throws IOException { + when(bucket.getFileStatus(anyString(), anyBoolean())) + .thenThrow(new OMException("missing", + OMException.ResultCodes.FILE_NOT_FOUND)); + assertThrows(FileNotFoundException.class, + () -> adapter.getFileStatus("key", URI_O3FS, WORKING_DIR, "user", true)); + } + + @Test + public void otherOMExceptionPropagates() throws IOException { + when(bucket.getFileStatus(anyString(), anyBoolean())) + .thenThrow(new OMException("boom", + OMException.ResultCodes.INTERNAL_ERROR)); + assertThrows(OMException.class, + () -> adapter.getFileStatus("key", URI_O3FS, WORKING_DIR, "user", true)); + } +} diff --git a/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemHeadOp.java b/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemHeadOp.java new file mode 100644 index 000000000000..d0dd8084394a --- /dev/null +++ b/hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemHeadOp.java @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.ozone; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.net.URI; +import org.apache.hadoop.fs.BlockLocation; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +/** + * Unit tests for the head-op (metadata-only) type checks on o3fs + * ({@link BasicOzoneFileSystem#isDirectory}/{@link BasicOzoneFileSystem#isFile}) + * added in HDDS-15877. Uses a mock adapter so no cluster is required. + */ +public class TestOzoneFileSystemHeadOp { + + private BasicOzoneClientAdapterImpl adapter; + private BasicOzoneFileSystem fs; + + /** Test FS that injects a mock adapter instead of connecting to OM. */ + private final class MockAdapterFs extends BasicOzoneFileSystem { + @Override + protected OzoneClientAdapter createAdapter(ConfigurationSource conf, + String bucketStr, String volumeStr, String omHost, int omPort) { + return adapter; + } + } + + @BeforeEach + public void setUp() throws IOException { + adapter = mock(BasicOzoneClientAdapterImpl.class); + fs = new MockAdapterFs(); + fs.initialize(URI.create("o3fs://bucket.vol/"), new OzoneConfiguration()); + } + + private static FileStatusAdapter status(Path path, boolean isDir) { + return new FileStatusAdapter(0L, 0L, path, isDir, (short) 3, 0L, 0L, 0L, + (short) 0, "user", "group", null, new BlockLocation[0], false, false); + } + + private void stubStatus(boolean isDir) throws IOException { + when(adapter.getFileStatus(anyString(), any(URI.class), any(Path.class), + anyString(), anyBoolean())) + .thenAnswer(inv -> status(inv.getArgument(2), isDir)); + } + + private void stubThrow(IOException e) throws IOException { + when(adapter.getFileStatus(anyString(), any(URI.class), any(Path.class), + anyString(), anyBoolean())).thenThrow(e); + } + + @Test + public void isDirectoryUsesHeadOp() throws IOException { + stubStatus(true); + Path dir = new Path("/dir"); + + assertTrue(fs.isDirectory(dir)); + assertFalse(fs.isFile(dir)); + + ArgumentCaptor headOp = ArgumentCaptor.forClass(Boolean.class); + verify(adapter, atLeastOnce()).getFileStatus( + anyString(), any(URI.class), any(Path.class), anyString(), + headOp.capture()); + for (Boolean v : headOp.getAllValues()) { + assertTrue(v, "isDirectory/isFile must request headOp"); + } + } + + @Test + public void isFileUsesHeadOp() throws IOException { + stubStatus(false); + Path file = new Path("/file"); + + assertTrue(fs.isFile(file)); + assertFalse(fs.isDirectory(file)); + } + + @Test + public void fullGetFileStatusDoesNotUseHeadOp() throws IOException { + stubStatus(false); + fs.getFileStatus(new Path("/file")); + verify(adapter).getFileStatus(anyString(), any(URI.class), any(Path.class), + anyString(), eq(false)); + } + + @Test + public void missingPathReturnsFalse() throws IOException { + // The adapter maps FILE_NOT_FOUND to FileNotFoundException; the FS maps + // KEY_NOT_FOUND to FileNotFoundException. Both are swallowed as "false". + stubThrow(new FileNotFoundException("missing")); + Path missing = new Path("/missing"); + assertFalse(fs.isDirectory(missing)); + assertFalse(fs.isFile(missing)); + + stubThrow(new OMException("missing", + OMException.ResultCodes.KEY_NOT_FOUND)); + assertFalse(fs.isDirectory(missing)); + assertFalse(fs.isFile(missing)); + } + + @Test + public void otherOMExceptionPropagates() throws IOException { + stubThrow(new OMException("denied", + OMException.ResultCodes.PERMISSION_DENIED)); + assertThrows(OMException.class, + () -> fs.isDirectory(new Path("/x"))); + } + + @Test + public void plainIOExceptionPropagates() throws IOException { + stubThrow(new IOException("io")); + assertThrows(IOException.class, + () -> fs.isFile(new Path("/x"))); + } +}