HDDS-16326. Cache the container chunks directory instead of stat-ing it on every chunk operation - #11151
HDDS-16326. Cache the container chunks directory instead of stat-ing it on every chunk operation#11151rich7420 wants to merge 3 commits into
Conversation
…it on every chunk operation ContainerUtils.getChunkDir built a new File and called exists() on the container chunks directory on every chunk read/write. Resolve and validate it once, cache the File on ContainerData, and reuse it on the data plane; setChunksPath invalidates the cache. A chunks directory that disappears mid-life is still caught by the subsequent open(), so volume-failure handling is preserved. The container scanner stats the path independently and is unaffected. Behavior unchanged. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
|
Quick JMH of a page-cache-hot The removed work is ~one stat plus a |
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for working on this! @rich7420 I noticed that TestContainerStateMachineFailures fails, and I reproduced it locally. I think this change currently breaks storage-failure detection. Could you take a look? Thanks!
https://github.com/apache/ozone/actions/runs/33231158748/job/99045395282#step:13:5472
The first version cached the chunks directory in getChunkDir for every caller, which dropped the per-op existence check that storage-failure detection relies on. TestContainerStateMachineFailures#testUnhealthyContainer deletes the chunks directory and expects the next write to mark the container UNHEALTHY; with the cached directory the write no longer surfaced the missing volume, so the container stayed healthy. Restore getChunkDir to validate (stat) on every call so writes and other operations detect a missing directory as before, and move the cache behind a new getChunkDirForRead / getChunkFileForRead used only by the readChunk path. Reads keep the per-op stat savings while writes keep the failure probe. TestContainerUtils now covers both paths: getChunkDir re-detects a deleted directory, getChunkDirForRead caches the resolved directory. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
Fold the read-path cache into a single ContainerData#getChunksDirForRead that resolves once via ContainerUtils.getChunkDir and reuses the container's private chunksDirFile field, and drop the parallel getChunkDirForRead and getChunkFileForRead wrappers added earlier. readChunk now resolves through the existing ContainerLayoutVersion#getChunkFile(File, BlockID, String) overload. No behavior change: writes and other callers still validate on every getChunkDir, reads still skip the repeated stat. The public getChunksDirFile / setChunksDirFile pair is gone; the cache stays private to its owner. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
|
@chihsuan thanks for the reminder |
|
Thanks @chihsuan. The |
| * @return the resolved chunks directory | ||
| * @throws StorageContainerException if the chunks directory cannot be resolved | ||
| */ | ||
| public File getChunksDirForRead() throws StorageContainerException { |
There was a problem hiding this comment.
Should this getter be @JsonIgnore?
| // 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()); |
There was a problem hiding this comment.
Should we still validate the directory at this path to preserve the previous error and logging behavior?
| * 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. |
There was a problem hiding this comment.
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.
What changes were proposed in this pull request?
ContainerUtils.getChunkDirbuilt anew File(chunksPath)and calledexists()on the container's chunks directory on every WriteChunk and ReadChunk (reached viaContainerLayoutVersion.getChunkFile). The chunks directory is fixed for the container's lifetime, so on the read path this stat is pure added work.The read path now resolves the directory once and caches it:
ContainerData.getChunksDirForRead()validates viagetChunkDiron first use, stores the result in atransient volatile Filefield, and reuses it for the container's lifetime.ReadChunkin both layout strategies uses it.Writes and every other caller keep going through
getChunkDir, which still stats on every call. That per-op check is load-bearing: it is how a WriteChunk against a failed volume surfaces asUNABLE_TO_FIND_DATA_DIRso the container is marked UNHEALTHY (covered byTestContainerStateMachineFailures#testUnhealthyContainer). An earlier revision cached it away for writes too and broke that detection; keeping the stat on writes preserves it.On the read path the eager probe is not needed: a directory that disappears after it was cached is caught by the read's own
open(), and a failed read does not mark the container UNHEALTHY (read-side volume failures are handled out of band by the volume checker).setChunksPathinvalidates the cache so create / import / move re-resolve, and the field istransient, so it is never serialized to the container YAML. The container scanner (KeyValueContainerCheck) stats the path independently and is unaffected.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16326
How was this patch tested?
TestContainerUtils(read cache + write re-validation) andTestContainerStateMachineFailures#testUnhealthyContainer(storage-failure detection on write), plus a green fork CI run: https://github.com/rich7420/ozone/actions/runs/33411361078