-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add read-only Engine #33563
Add read-only Engine #33563
Conversation
This change adds an engine implementation that opens a reader on an existing index but doesn't permit any refreshes or modifications to the index. Relates to elastic#32867 Relates to elastic#32844
Pinging @elastic/es-distributed |
@dnhatn FYI I broken this out of your and my PR since they share pretty much the same functionality into it's own PR |
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
@Override | ||
public SyncedFlushResult syncFlush(String syncId, CommitId expectedCommitId) throws EngineException { | ||
// we can't do synced flushes this would require an indexWriter which we don't have | ||
return SyncedFlushResult.COMMIT_MISMATCH; |
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.
hmm... I really wonder if we are better off throwing in an exception here...
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.
yeah lets do that. I think we can cope with it.
this(config, null, new TranslogStats(0, 0, 0, 0, 0), true); | ||
} | ||
|
||
ReadOnlyEngine(EngineConfig config, SeqNoStats seqNoStats, TranslogStats translogStats, boolean obtainLock) { |
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.
java docs on obtainLock please.
import java.util.stream.Stream; | ||
|
||
/** | ||
* A basic read-only engine that allows switching a shard to be true read-only temporarily or permanently. |
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.
add a note that this be opened concurrently with another engine? It think that's not obvious.
} | ||
|
||
@Override | ||
public void rollTranslogGeneration() throws EngineException { throw new UnsupportedOperationException(); } |
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.
new lines missing. Also we should assert this is never called. Also no need for throws EngineException
|
||
@Override | ||
public IndexResult index(Index index) { | ||
throw new UnsupportedOperationException(); |
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.
we should assert this is never called (same for the other places here where UnsupportedOperationException
is thrown), as this indicates a bug.
return ElasticsearchDirectoryReader.wrap(reader, shardId); | ||
} | ||
|
||
public static SeqNoStats buildSeqNoStats(SegmentInfos infos) { |
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.
why make this public?
|
||
@Override | ||
public Closeable acquireRetentionLockForPeerRecovery() { | ||
return () -> {}; |
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 think this should be an assert false;
+ throw new UnsupportedOperationException
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.
it's totally fine to do a peerRecovery from this engine.
} | ||
|
||
@Override | ||
public void waitForOpsToComplete(long seqNo) { } |
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.
add newline
} | ||
|
||
@Override | ||
public void refresh(String source) throws EngineException {} |
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.
newline, also no need to write throws EngineException
public void refresh(String source) throws EngineException {} | ||
|
||
@Override | ||
public void writeIndexingBuffer() throws EngineException {} |
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.
newline
also no need to write throws EngineException
} | ||
|
||
@Override | ||
public CommitId flush(boolean force, boolean waitIfOngoing) throws EngineException { |
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.
no need for throws EngineException
public void trimOperationsFromTranslog(long belowTerm, long aboveSeqNo) throws EngineException { } | ||
|
||
@Override | ||
public void maybePruneDeletes() { } |
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.
newline
} | ||
|
||
@Override | ||
public void trimOperationsFromTranslog(long belowTerm, long aboveSeqNo) throws EngineException { } |
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.
newline, no need for throws EngineException
* master: Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568)
* master: (91 commits) Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568) Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) ...
This change adds an engine implementation that opens a reader on an
existing index but doesn't permit any refreshes or modifications
to the index.
Relates to #32867
Relates to #32844