Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,10 @@ public static File getContainerFile(File containerBaseDir) {
}

/**
* Get the chunk directory from the containerData.
* Resolve and validate the chunk directory from the containerData. The
* directory's existence is checked on every call, so a chunk operation
* against a failed volume surfaces as a storage failure; write paths rely on
* this to mark the container unhealthy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could this Javadoc stay about what the method does? It now describes what callers rely on, and the same note appears in other files.

*
* @param containerData {@link ContainerData}
* @return the file of chunk directory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import jakarta.annotation.Nullable;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
Expand All @@ -47,6 +48,7 @@
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerType;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
import org.apache.hadoop.ozone.container.common.helpers.ContainerUtils;
import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
import org.apache.ratis.util.Preconditions;
Expand Down Expand Up @@ -77,6 +79,10 @@ public abstract class ContainerData {
// Path to Physical file system where chunks are stored.
private String chunksPath;

// Chunks directory resolved and validated once from chunksPath, cached to
// avoid re-stat-ing it on every chunk operation. Not serialized.
private transient volatile File chunksDirFile;

// State of the Container
private ContainerDataProto.State state;

Expand Down Expand Up @@ -263,6 +269,26 @@ public String getChunksPath() {
*/
public void setChunksPath(String chunkPath) {
this.chunksPath = chunkPath;
this.chunksDirFile = null;
}

/**
* Read-path accessor for the chunks directory. Resolves and validates it once
* via {@link ContainerUtils#getChunkDir(ContainerData)}, then returns the
* cached result on later calls to skip the per-read stat. Writes and other
* callers use {@code ContainerUtils.getChunkDir} directly, so a missing
* directory still surfaces as a storage failure on every operation.
*
* @return the resolved chunks directory
* @throws StorageContainerException if the chunks directory cannot be resolved
*/
public File getChunksDirForRead() throws StorageContainerException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this getter be @JsonIgnore?

File dir = chunksDirFile;
if (dir == null) {
dir = ContainerUtils.getChunkDir(this);
chunksDirFile = dir;
}
return dir;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ public ChunkBufferToByteString readChunk(Container container, BlockID blockID,

HddsVolume volume = containerData.getVolume();

final File chunkFile = getChunkFile(container, blockID);
// Reads use the cached chunks directory to skip the per-read stat; writes
// keep validating via getChunkFile so a missing directory is still detected.
final File chunkFile = FILE_PER_BLOCK.getChunkFile(containerData.getChunksDirForRead(), blockID, null);

final long len = info.getLen();
long offset = info.getOffset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,10 @@ public ChunkBufferToByteString readChunk(Container container, BlockID blockID,
HddsVolume volume = containerData.getVolume();

// In version1, we verify checksum if it is available and return data
// of the chunk file.
File finalChunkFile = getChunkFile(kvContainer, blockID, info);
// of the chunk file. Reads use the cached chunks directory to skip the
// per-read stat; writes keep validating so a missing directory is detected.
File finalChunkFile = FILE_PER_CHUNK.getChunkFile(
containerData.getChunksDirForRead(), blockID, info.getChunkName());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still validate the directory at this path to preserve the previous error and logging behavior?


List<File> possibleFiles = new ArrayList<>();
possibleFiles.add(finalChunkFile);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static org.apache.hadoop.ozone.container.ContainerTestHelper.getDummyCommandRequestProto;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -55,8 +56,10 @@
import org.apache.hadoop.hdds.scm.ScmConfigKeys;
import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
import org.apache.hadoop.ozone.common.ChunkBuffer;
import org.apache.hadoop.ozone.container.common.impl.ContainerLayoutVersion;
import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
import org.apache.hadoop.ozone.container.common.volume.VolumeInfoMetrics;
import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
import org.apache.ratis.thirdparty.com.google.protobuf.TextFormat;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -76,6 +79,65 @@ void setup(@TempDir File dir) {
conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, dir.toString());
}

@Test
void getChunkDirReDetectsMissingDirectory(@TempDir File dir) throws Exception {
File chunks = new File(dir, "chunks");
assertTrue(chunks.mkdirs());

KeyValueContainerData data = new KeyValueContainerData(1L,
ContainerLayoutVersion.FILE_PER_BLOCK, 1024L * 1024 * 1024,
UUID.randomUUID().toString(), UUID.randomUUID().toString());
data.setChunksPath(chunks.getAbsolutePath());

// getChunkDir validates on every call, so a write against a failed volume
// is detected even after the directory was resolved once.
assertEquals(chunks.getAbsolutePath(),
ContainerUtils.getChunkDir(data).getAbsolutePath());
assertTrue(chunks.delete());
StorageContainerException e = assertThrows(StorageContainerException.class,
() -> ContainerUtils.getChunkDir(data));
assertEquals(Result.UNABLE_TO_FIND_DATA_DIR, e.getResult());
}

@Test
void getChunksDirForReadCachesResolvedDirectory(@TempDir File dir) throws Exception {
File chunks = new File(dir, "chunks");
assertTrue(chunks.mkdirs());

KeyValueContainerData data = new KeyValueContainerData(1L,
ContainerLayoutVersion.FILE_PER_BLOCK, 1024L * 1024 * 1024,
UUID.randomUUID().toString(), UUID.randomUUID().toString());
data.setChunksPath(chunks.getAbsolutePath());

// Resolved and validated once, then cached: same instance on every read.
File first = data.getChunksDirForRead();
assertSame(first, data.getChunksDirForRead());

// The read cache survives the directory disappearing; a mid-life
// disappearance is caught by the read's own open(), not by this method.
assertTrue(chunks.delete());
assertSame(first, data.getChunksDirForRead());

// Changing the path invalidates the cache and re-resolves.
File other = new File(dir, "chunks2");
assertTrue(other.mkdirs());
data.setChunksPath(other.getAbsolutePath());
assertEquals(other.getAbsolutePath(),
data.getChunksDirForRead().getAbsolutePath());
}

@Test
void getChunkDirThrowsWhenChunksDirMissing(@TempDir File dir) {
KeyValueContainerData data = new KeyValueContainerData(1L,
ContainerLayoutVersion.FILE_PER_BLOCK, 1024L * 1024 * 1024,
UUID.randomUUID().toString(), UUID.randomUUID().toString());
data.setChunksPath(new File(dir, "missing").getAbsolutePath());

StorageContainerException e = assertThrows(StorageContainerException.class,
() -> ContainerUtils.getChunkDir(data));
assertEquals(Result.UNABLE_TO_FIND_DATA_DIR, e.getResult());
}

@Test
public void redactsDataBuffers() {
// GIVEN
Expand Down
Loading