Skip to content
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

Merged
merged 5 commits into from
Sep 11, 2018
Merged

Add read-only Engine #33563

merged 5 commits into from
Sep 11, 2018

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Sep 10, 2018

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

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
@s1monw s1monw added >enhancement v7.0.0 :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. v6.5.0 labels Sep 10, 2018
@s1monw s1monw requested a review from bleskes September 10, 2018 12:13
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@s1monw
Copy link
Contributor Author

s1monw commented Sep 10, 2018

@dnhatn FYI I broken this out of your and my PR since they share pretty much the same functionality into it's own PR

Copy link
Contributor

@bleskes bleskes left a 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;
Copy link
Contributor

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...

Copy link
Contributor Author

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) {
Copy link
Contributor

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.
Copy link
Contributor

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(); }
Copy link
Contributor

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();
Copy link
Contributor

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) {
Copy link
Contributor

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 () -> {};
Copy link
Contributor

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

Copy link
Contributor Author

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) { }
Copy link
Contributor

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 {}
Copy link
Contributor

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 {}
Copy link
Contributor

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 {
Copy link
Contributor

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() { }
Copy link
Contributor

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 { }
Copy link
Contributor

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

@s1monw s1monw merged commit 517cfc3 into elastic:master Sep 11, 2018
@s1monw s1monw deleted the add_read_only_engine branch September 11, 2018 12:05
s1monw added a commit that referenced this pull request Sep 11, 2018
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
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 11, 2018
* 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)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 11, 2018
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants