Add a NoopEngine implementation by dakrone · Pull Request #31163 · elastic/elasticsearch

dakrone · 2018-06-07T00:16:45Z

This adds a new Engine implementation that does.. nothing. Any operations throw
an UnsupportedOperationException when tried. This engine is intended as a
building block for replicated closed indices in subsequent code changes.

Relates to #31141

This adds a new Engine implementation that does.. nothing. Any operations throw an `UnsupportedOperationException` when tried. This engine is intended as a building block for replicated closed indices in subsequent code changes. Relates to elastic#31141

elasticmachine · 2018-06-07T00:16:47Z

Pinging @elastic/es-distributed

jasontedor

I left some high-level comments. I have not done a careful review.

jasontedor · 2018-06-07T00:21:12Z

server/src/main/java/org/elasticsearch/index/engine/Engine.java

+    /**
+     * Returns true if the engine is a noop engine
+     */
+    public abstract boolean isNoopEngine();


This feels wrong to me. It looks like it's only used for testing? Is this really needed? We did not feel the need to add something like this for the following engine in CCR.

Sorry this snuck in from me extracting this from my other branch (where this is actually used), I'll remove it for now and re-add it for the next set of work

jasontedor · 2018-06-07T00:23:01Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngineFactory.java

    }
+
+    @Override
+    public Engine newNoopEngine(EngineConfig config) {


Can you take a look at how we handle the following engine in CCR and see if that extension point makes sense instead of adding this here? It feels conceptually wrong that the internal engine factory returns something other than an internal engine.

Do we want to make this a plugin? I was thinking built-in but I guess we could move it to a module if we wanted to, I wasn't sure if it's a good idea to do engine implementations inside of modules

Looking at this again, it appears that EnginePlugin is part of the CCR branch, are there plans to factor it out into a separate PR that goes into master?

I think it would be great if we could make it a module, I am always happy to see isolation when it makes sense and here I think it can. Note that CCR has an engine implementation inside a module. 😇

We have talked exactly about the possibility of reusing the engine plugin work in the context of closed indices many months ago so we could bring that work out of the CCR branch and into 6.x/master.

Okay, I've removed the changes to the EngineFactory and I can use the pluggable one for the next PR (since this one doesn't hook the engine in yet)

s1monw

left some comments.

s1monw · 2018-06-07T19:31:08Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+
+    @Override
+    public void restoreLocalCheckpointFromTranslog() {
+


s1monw · 2018-06-07T19:32:15Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+        Translog translog = null;
+
+        // The deletion policy for the translog should not keep any translogs around, so the min age/size is set to -1
+        final TranslogDeletionPolicy translogDeletionPolicy = new TranslogDeletionPolicy(-1, -1);


can we move this into the try block please?

s1monw · 2018-06-07T19:32:55Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+    private final IndexCommit lastCommit;
+    private final LocalCheckpointTracker localCheckpointTracker;
+    private final String historyUUID;
+    private SegmentInfos lastCommittedSegmentInfos;


this can be final no?

s1monw · 2018-06-07T19:33:09Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+ * Directory so that the last commit's user data can be read for the historyUUID
+ * and last committed segment info.
+ */
+public class NoopEngine extends Engine {


can this class be final and maybe pkg private?

Sure, though I'll un-final it when I work on the second half of this, but it can be final for now :)

s1monw · 2018-06-07T19:34:07Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+    @Override
+    public CommitId flush() throws EngineException {
+        try {
+            translog.rollGeneration();


this looks wrong. We don't write anything here why do we need to modify the translog? I think this should be read-only

In order for flushing to clear existing translogs (because we don't want any translog operations to be replayed for peer or store recovery) we want the flush method to remove the translog, this was added so that flushing the new engine would ensure that we don't have any translog operations around that could cause UOEs during recovery

I can remove this for now and re-introduce it later when adding the state transition part, if that makes it better, but we still need to be able to completely remove translog ops before doing recovery since we have no way to do operation-based recovery.

What do you think?

yeah I'd like to remove it for now I can 't see in this change why it's needed

Okay, I've removed that change from this PR

bleskes

Thanks @dakrone . I left some comments and questions.

bleskes · 2018-06-08T17:06:31Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            translogDeletionPolicy.setTranslogGenerationOfLastCommit(lastGen);
+            translogDeletionPolicy.setMinTranslogGenerationForRecovery(lastGen);
+
+            localCheckpointTracker = createLocalCheckpointTracker();


we don't really need a local checkpoint tracker here, instead can we work on master to not expose the localCheckpointTracker out of engine (similar to how we don't expose the translog) and then we can avoid creating it? We should assert that maxSeq == localCheckpoint when opening lucene and otherwise fail.

Do you mean something like this: #31213

yes. thanks.

bleskes · 2018-06-08T17:07:13Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            final TranslogDeletionPolicy translogDeletionPolicy = new TranslogDeletionPolicy(-1, -1);
+
+            lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo();
+            translog = openTranslog(engineConfig, translogDeletionPolicy, engineConfig.getGlobalCheckpointSupplier());


Do we really need a translog? all we want is to validate that the translog has a the right uuid and that it's empty?

We use the translog all over the place in other methods in Engine. We can try to encapsulate all of these into different methods (similar to #31213), but I'm not sure what benefit we'd actually get from that. In the next iteration we'll need the translog because we'll need to sync and flush it on engine opening so that there are no operations to be replayed during peer recovery (which I think will still need a translog to retrieve stats about # of ops).

I'm not sure what benefit we'd actually get from that

I looked at these methods and I agree it's on the edge - there are quite a few. I would still prefer we replace them with unsupported operations or noop returns (ex empty sanpshots). This will lock things down and prevent things that aren't supposed to happen - I think that's good. An alternative is to implement a NoopTranslog but that's another rabbit hole.

we'll need to sync and flush it on engine opening

Why is that? I think it's good to only close indices that have no ongoing indexing (like our plan for frozen index). Regardless - why can't we do the flush / trim when we close the open engine and convert it to a noop engine?

I can see one thing down the road because we may close an index on recovery where it has broken settings (TBD). In that case I would still prefer to make utilities methods like Store#associateIndexWithNewTranslog that work on the folder. Note that this problem isn't solved even if we keep the translog open as we can't index operations from it into the lucene index with the NoopEngine nor ship to a recovering shard with NoopEngine in it. We assume those don't exist. I think we should discuss this separately.

I think it's good to only close indices that have no ongoing indexing (like our plan for frozen index).

Yes, but even with no ongoing indexing, a translog still remains (due to retention policy)

Regardless - why can't we do the flush / trim when we close the open engine and convert it to a noop engine?

That would require setting a new retention policy on an existing engine (making a part of InternalEngine mutable which makes me :(). In the future though, we could do the retention policy, sync, and flush/trim when the NoopEngine is opened, and then immediately close the translog.

In order to do this though, we'll have to remove the getTranslog method from Engine, is that something you want me to do as a precursor to this?

bleskes · 2018-06-08T17:08:02Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+
+    @Override
+    public boolean ensureTranslogSynced(Stream<Translog.Location> locations) {
+        return false;


I think this can throw an unsupported operation exception too?

bleskes · 2018-06-08T19:03:21Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+    }
+
+    @Override
+    public Translog getTranslog() {


this increases visibilty

dakrone · 2018-06-11T18:00:21Z

@jasontedor @bleskes @s1monw I've removed almost all the pieces from this that were deemed unnecessary, can you take another look please?

bleskes

Looks great thanks for the extra iteration. I left some minor asks.

bleskes · 2018-06-11T18:27:10Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            lastCommit = indexCommits.get(indexCommits.size()-1);
+            historyUUID = lastCommit.getUserData().get(HISTORY_UUID_KEY);
+            // We don't want any translogs hanging around for recovery, so we need to set these accordingly
+            final long lastGen = Long.parseLong(lastCommit.getUserData().get(Translog.TRANSLOG_GENERATION_KEY));


this is not used?

Also - I think it's to validate integrity - i.e. open the translog, see that it's uuid matches, see that it's empty and shut it down?

Okay I pushed a commit removing the unused lastGen and open-closing the translog for validation purposes

bleskes · 2018-06-11T18:27:59Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+
+    @Override
+    public SeqNoStats getSeqNoStats(long globalCheckpoint) {
+        return new SeqNoStats(0, 0, 0);


use the incoming globalCheckpoint?

Wouldn't that violate the assertion that global checkpoint is the minimum of all shards' local checkpoints?

that would but we don't check this here. Also - I think we should load the stats from the last commit point, not just 0 (both max and local checkpoint are stored in the commit)

bleskes

I did another round. Sorry for not thinking about these in the first run.

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            final TranslogDeletionPolicy translogDeletionPolicy = new TranslogDeletionPolicy(-1, -1);
+
+            // The translog is opened and closed to validate that the translog UUID from lucene is the same as the one in the translog
+            Translog translog = openTranslog(engineConfig, translogDeletionPolicy, engineConfig.getGlobalCheckpointSupplier());


bleskes · 2018-06-11T19:53:27Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+
+    @Override
+    public SeqNoStats getSeqNoStats(long globalCheckpoint) {
+        return new SeqNoStats(0, 0, 0);


that would but we don't check this here. Also - I think we should load the stats from the last commit point, not just 0 (both max and local checkpoint are stored in the commit)

bleskes · 2018-06-11T19:53:52Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+    }
+
+    @Override
+    public void resetLocalCheckpoint(long localCheckpoint) {


can you assert this is equal to getLocalCheckpoint()?

Yep added this

bleskes · 2018-06-11T19:54:03Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+    }
+
+    @Override
+    public long getLocalCheckpoint() {


can you return the local checkpoint stored in the commit?

Sure, added this

bleskes · 2018-06-11T19:55:28Z

server/src/test/java/org/elasticsearch/index/engine/NoopEngineTests.java

+
+public class NoopEngineTests extends EngineTestCase {
+    private static final IndexSettings INDEX_SETTINGS = IndexSettingsModule.newIndexSettings("index", Settings.EMPTY);
+


can you also add a test that creates a normal engine, adds some data, flushes, closes and the opens a noop engine and verify all the stats makes sense and getting a lucene snapshot returns the expected data (doc counts etc)?

getting a lucene snapshot returns the expected data (doc counts etc)?

Do you mean from newTranslogSnapshotFromMinSeqNo? I've been returning an empty snapshot for this with docCount of 0 (and no operations, because we have no way of returning operations), so it won't match the same doc count that a snapshot from a regular engine will.

I meant this . The standard engine ends up returning a SnapshotIndexCommit which is why I used the term lucene snapshot. I hope this is clearer.

Yep, thanks for clarifying, this was added as a test

dakrone · 2018-06-12T14:41:28Z

@bleskes thanks for taking another look, I think I addressed all your feedback

bleskes

LGTM. Thanks for the extra work @dakrone . I left some question on the test, but I'm happy either way.

bleskes · 2018-06-12T20:22:19Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

@@ -98,6 +98,9 @@ public void close() {

            // The translog is opened and closed to validate that the translog UUID from lucene is the same as the one in the translog
            Translog translog = openTranslog(engineConfig, translogDeletionPolicy, engineConfig.getGlobalCheckpointSupplier());


wrap this in try finally?

Yes good catch, I thought about that last night but forgot to do it, I'll update that

bleskes · 2018-06-12T20:39:47Z

server/src/test/java/org/elasticsearch/index/engine/NoopEngineTests.java


+    public void testNoopAfterRegularEngine() throws IOException {
+        int docs = randomIntBetween(1, 10);
+        ReplicationTracker tracker = (ReplicationTracker) engine.config().getGlobalCheckpointSupplier();


why is this needed? we typically just write to the engine?

This is a rabbit hole @jasontedor and I got into last night; in order to completely flush and remove the translog we need a deletion policy where we can advance the generation needed for recovery, in order to advance that we have to have the tracker working correctly (which I guess is normally managed at the IndexShard level), so this allows us to advance global checkpoints as needed so we can remove the translog and open a new noop engine (otherwise the noop engine would throw an exception about there being translog operations when being opened).

We can pass the global checkpoint as a LongSupplier to an engine.

final AtomicLong globalCheckpoint = new AtomicLong(SequenceNumbers.NO_OPS_PERFORMED); try (Store store = createStore(); InternalEngine engine = createEngine(store, createTempDir(), globalCheckpoint::get)) { engine.syncTranslog(); // Make sure that the global checkpoint is persisted to the translog's checkpoint }

bleskes · 2018-06-12T20:41:44Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            List<IndexCommit> indexCommits = DirectoryReader.listCommits(store.directory());
+            lastCommit = indexCommits.get(indexCommits.size()-1);
+            historyUUID = lastCommit.getUserData().get(HISTORY_UUID_KEY);
+            localCheckpoint = Long.parseLong(lastCommit.getUserData().get(SequenceNumbers.LOCAL_CHECKPOINT_KEY));


This is not yet relevant to this PR but these will have to be the same IMO .

jasontedor

I left some comments/questions.

jasontedor · 2018-06-13T01:08:17Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+        try {
+            lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo();
+            List<IndexCommit> indexCommits = DirectoryReader.listCommits(store.directory());
+            lastCommit = indexCommits.get(indexCommits.size()-1);


Formatting nit: indexCommits.size()-1 -> indexCommits.size() - 1

jasontedor · 2018-06-13T01:09:30Z

server/src/main/java/org/elasticsearch/index/engine/NoopEngine.java

+            // The deletion policy for the translog should not keep any translogs around, so the min age/size is set to -1
+            final TranslogDeletionPolicy translogDeletionPolicy = new TranslogDeletionPolicy(-1, -1);
+
+            // The translog is opened and closed to validate that the translog UUID from lucene is the same as the one in the translog


Can we have a test case for this?

Sure, added a test

jasontedor · 2018-06-13T01:11:00Z

server/src/test/java/org/elasticsearch/index/engine/NoopEngineTests.java

+        final NoopEngine engine = new NoopEngine(noopConfig(INDEX_SETTINGS, store, primaryTranslogDir));
+        expectThrows(UnsupportedOperationException.class, () -> engine.index(null));
+        expectThrows(UnsupportedOperationException.class, () -> engine.delete(null));
+        expectThrows(UnsupportedOperationException.class, () -> engine.noOp(null));


As a follow-up can we have:

Noop -> NoOp

noop -> no-op (when written in exception messages to the user, and code comments)

noopEngine -> `noOpEngine

jasontedor · 2018-06-13T01:11:23Z

server/src/test/java/org/elasticsearch/index/engine/NoopEngineTests.java

+
+    public void testTwoNoopEngines() throws IOException {
+        engine.close();
+        // It's so noop you can even open two engines for the same store without tripping anything


Is this something that we are going to rely on?

Probably not? I just thought it was a good test that we aren't acquiring locks or locking files accidentally or anything like that

Okay, maybe the test name should be clearer, or at least a comment of its purpose?

Sure I'll add a comment

dakrone · 2018-06-13T17:55:19Z

@jasontedor I addressed your comments (I'll do the rename in a followup)

jasontedor

LGTM.

dakrone · 2018-06-14T21:42:39Z

@elasticmachine run sample packaging tests please

In elastic#31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to elastic#31141

In #31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to #31141

This commit uses the NoOp engine introduced in elastic#31163 for closed indices. Instead of being removed from the routing table and essentially "dropped" (but not deleted), closed indices are now replicated the same way open indices are. Relates to elastic#31141

This adds a new Engine implementation that does.. nothing. Any operations throw an `UnsupportedOperationException` when tried. This engine is intended as a building block for replicated closed indices in subsequent code changes. Relates to #31141

In #31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to #31141

This adds a new Engine implementation that does.. nothing. Any operations throw an `UnsupportedOperationException` when tried. This engine is intended as a building block for replicated closed indices in subsequent code changes. Relates to elastic#31141

In elastic#31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to elastic#31141

Add a NoopEngine implementation

c345e54

This adds a new Engine implementation that does.. nothing. Any operations throw an `UnsupportedOperationException` when tried. This engine is intended as a building block for replicated closed indices in subsequent code changes. Relates to elastic#31141

dakrone added >non-issue :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels Jun 7, 2018

jasontedor requested changes Jun 7, 2018

View reviewed changes

dakrone added 3 commits June 6, 2018 18:25

Remove isNoopEngine() method

7d469cc

Remove test using isNoopEngine

5c0ed00

Remove additional EngineFactory method

2985692

dakrone requested a review from s1monw June 7, 2018 18:04

s1monw suggested changes Jun 7, 2018

View reviewed changes

dakrone added 6 commits June 7, 2018 13:41

Remove newline

19b50b0

Move translog deletion policy creation into try block

7d8e9de

Make lastCommittedSegmentInfos final

21de3a4

Make NoopEngine final and package private

ce634e9

Remove translog flushing

e49a854

Fix checkstyle

2629329

bleskes suggested changes Jun 8, 2018

View reviewed changes

Throw UOE from ensureTranslogSynced

191eae0

bleskes reviewed Jun 8, 2018

View reviewed changes

Reduce visibility of getTranslog

5ed1bae

dakrone mentioned this pull request Jun 8, 2018

Replicate closed indices #31141

Closed

8 tasks

dakrone added v7.0.0 and removed v7.0.0 labels Jun 8, 2018

dakrone added 2 commits June 11, 2018 10:38

Merge remote-tracking branch 'origin/master' into add-noop-engine

6931981

Get rid of LocalCheckpointTracker and Translog in NoopEngine

8f59a7e

bleskes suggested changes Jun 11, 2018

View reviewed changes

Remove unused lastGen, validate Translog UUID

de63a5e

bleskes suggested changes Jun 11, 2018

View reviewed changes

Read local checkpoint and max seq no out of commit data

c2c820a

dakrone added 2 commits June 11, 2018 16:18

Add check for translog operations and test

ba56f25

Fix checkstyle

023effb

bleskes approved these changes Jun 12, 2018

View reviewed changes

Use try-with-resources for translog opening

a4d2e3b

dakrone requested review from jasontedor and s1monw June 12, 2018 21:33

jasontedor reviewed Jun 13, 2018

View reviewed changes

s1monw approved these changes Jun 13, 2018

View reviewed changes

dakrone added 2 commits June 13, 2018 08:04

Formatting nit

78ae62d

Add a test for validating translog UUID

13c06b9

Merge remote-tracking branch 'origin/master' into add-noop-engine

4e428d2

jasontedor approved these changes Jun 14, 2018

View reviewed changes

Enhance comment about two noop engine test

37398ac

dakrone merged commit e1d068d into elastic:closed-index-replication Jun 15, 2018

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 18, 2018

Rename NoopEngine -> NoOpEngine

f86f71d

In elastic#31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to elastic#31141

dakrone mentioned this pull request Jun 18, 2018

Rename NoopEngine -> NoOpEngine #31398

Merged

dakrone added a commit that referenced this pull request Jun 19, 2018

Rename NoopEngine -> NoOpEngine (#31398)

fc63e6c

In #31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to #31141

dakrone mentioned this pull request Jul 2, 2018

Use NoOp engine for replicating closed indices #31745

Closed

tlrx pushed a commit that referenced this pull request Sep 14, 2018

Rename NoopEngine -> NoOpEngine (#31398)

5741915

In #31163 (comment) Jason requested renaming NoopEngine to NoOpEngine. This commit renames the class and relevant parts. Relates to #31141

tlrx mentioned this pull request Sep 20, 2018

[RCI] Add NoOpEngine for closed indices #33903

Merged

dakrone deleted the add-noop-engine branch February 4, 2019 14:47


		public class NoopEngineTests extends EngineTestCase {
		private static final IndexSettings INDEX_SETTINGS = IndexSettingsModule.newIndexSettings("index", Settings.EMPTY);

		@@ -98,6 +98,9 @@ public void close() {

		// The translog is opened and closed to validate that the translog UUID from lucene is the same as the one in the translog
		Translog translog = openTranslog(engineConfig, translogDeletionPolicy, engineConfig.getGlobalCheckpointSupplier());

Conversation

dakrone commented Jun 7, 2018

Uh oh!

elasticmachine commented Jun 7, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleskes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bleskes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone commented Jun 11, 2018 •

edited

Loading