Nuno Santos created OAK-12047:
---------------------------------
Summary: CachingSegmentArchiveReader#readSegment should check if
segment is present in archive before trying to load it from cache
Key: OAK-12047
URL: https://issues.apache.org/jira/browse/OAK-12047
Project: Jackrabbit Oak
Issue Type: Improvement
Components: segment-tar
Reporter: Nuno Santos
A single persistent disk cache instance is shared by all
CachingSegmentArchiveReaders of a segment store. This means it will contain
segments from all archives. {{CachingSegmentArchiveReaders.readSegment()}} is
checking if the segment is in the cache before checking if it is in the
archive. ([source
code|https://github.com/apache/jackrabbit-oak/blob/784d38ef842b16cb186f190de784c7da9842f607/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/spi/persistence/persistentcache/CachingSegmentArchiveReader.java#L48-L50]).
{code:java}
public Buffer readSegment(long msb, long lsb) throws IOException {
return persistentCache.readSegment(msb, lsb, () ->
delegate.readSegment(msb, lsb));
} {code}
If the segment is not in this archive, this behaviour is arguably wrong.
* *Segment not in cache* - This method will perform a lookup in the cache and
call {{readSegment}} on the delegate, which will return null, as the segment is
not in the archive. The current implementation of
[TarFiles.readSegment|https://github.com/apache/jackrabbit-oak/blob/784d38ef842b16cb186f190de784c7da9842f607/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarFiles.java#L635-L640]
iterates over all tar files sequentially, which means that the caching archive
reader will query the cache for each archive, even when the segment is not in
the archive. These additional lookups in the cache are wasteful and cause
noise. They artificially increase the cache miss metric (create noise in the
metrics), they add overhead, and they increase the contention on the underlying
cache, which can further degrade the performance.
* *Segment in cache* - The method will find the segment in the cache and
return it, even though the segment does not belong to this archive. This seems
to not cause any issue currently, but is a violation of the contract of the
method. {{readSegment()}} should return null if the segment is not in the
archive. With the current implementation of caching, the method may return null
or non-null depending on the contents of the cache.
This is a similar issue as OAK-12034.
The fix is simple: {{CachingSegmentArchiveReaders.readSegment()}} should first
check if the segment is in the archive before looking up in the cache. This is
a cheap operation as segment archives contain an index of segments.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
