-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Merge CacheDirectory into SearchableSnapshotDirectory #53917
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge CacheDirectory into SearchableSnapshotDirectory #53917
Conversation
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
import java.util.Set; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
public abstract class BaseSearchableSnapshotDirectory extends BaseDirectory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class has been merged down with SearchableSnapshotDirectory
(it was useful to me for the 2 previous PRs)
} | ||
|
||
@Nullable | ||
public IndexInputStats getStats(String fileName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comes from the now removed CacheDirectory and made public
as cache classes are in a different package
return new CacheBufferedIndexInput(this, fileInfo, context, inputStats); | ||
} else { | ||
long preferredLength = blobContainer.readBlobPreferredLength(); | ||
return new SearchableSnapshotIndexInput(blobContainer, fileInfo, context, preferredLength, BufferedIndexInput.BUFFER_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to rename this one to DirectBufferedIndexInput and to clean it up a bit in a follow up PR
@@ -47,13 +48,13 @@ | |||
// last seek position is kept around in order to detect forward/backward seeks for stats | |||
private long lastSeekPosition; | |||
|
|||
CacheBufferedIndexInput(CacheDirectory directory, FileInfo fileInfo, IOContext context, IndexInputStats stats) { | |||
public CacheBufferedIndexInput(SearchableSnapshotDirectory directory, FileInfo fileInfo, IOContext context, IndexInputStats stats) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has to be public
now it's instantiated by SearchableSnapshotDirectory
from a different package.
@@ -27,7 +27,7 @@ | |||
import java.util.concurrent.CompletableFuture; | |||
import java.util.concurrent.locks.ReentrantReadWriteLock; | |||
|
|||
class CacheFile { | |||
public class CacheFile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -18,7 +18,7 @@ | |||
private final ShardId shardId; | |||
private final String fileName; | |||
|
|||
CacheKey(SnapshotId snapshotId, IndexId indexId, ShardId shardId, String fileName) { | |||
public CacheKey(SnapshotId snapshotId, IndexId indexId, ShardId shardId, String fileName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And same here.
@@ -380,10 +405,86 @@ private void testIndexInputs(final CheckedBiConsumer<IndexInput, IndexInput, Exc | |||
}); | |||
} | |||
|
|||
public void testClearCache() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test comes from the now removed CacheDirectoryTests
class with very few adjustments for the SearchableSnapshotDirectory
instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks David |
Following #53860 and #53879, this pull request now merges
CacheDirectory
intoSearchableSnapshotDirectory
.I expect few more follow up pull requests, mostly to clean up things and move classes.