Simplify initialization of max_seq_no of updates #41161

dnhatn · 2019-04-12T18:09:04Z

Today we choose to initialize max_seq_no_of_updates on primaries only so we can deal with a situation where a primary is on an old node (before 6.5) which does not have MUS while replicas on new nodes (6.5+). However, this strategy is quite complex and can lead to bugs (for example #40249) since we have to assign a correct value (not too low) to MSU in all possible situations (before recovering from translog, restoring history on promotion, and handing off relocation).

Fortunately, we don't have to deal with this BWC in 7.0+ since all nodes in the cluster should have MSU. This change simplifies the initialization of MSU by always assigning it a correct value in the constructor of Engine regardless of whether it's a replica or primary.

Relates #33842

/cc @bleskes

elasticmachine · 2019-04-12T18:09:06Z

Pinging @elastic/es-distributed

henningandersen

@dnhatn , thanks for looking into this simplification, looking good.

I do wonder if we should move the field to InternalEngine, let me know your thoughts on that.

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

dnhatn · 2019-04-14T22:28:10Z

@henningandersen Thanks for looking. I pushed changes.

henningandersen

@dnhatn thanks for the additional work on this. Left two comments, hope you can clarify my uncertainty around advanceMaxSeqNoOfUpdatesOrDeletesOnPrimary?

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

henningandersen · 2019-04-15T10:38:49Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/index/engine/FollowingEngine.java

@@ -131,11 +131,6 @@ protected long generateSeqNoForOperationOnPrimary(final Operation operation) {
        return operation.seqNo();
    }

-    @Override


I am not sure about this, mainly because I do not understand why it was introduced in #39571 . I think it was done to make behavior identical to before changing the code to not mutate engine during planning step. Not sure if it is important though to not advance MSU here, since MSU should already be higher than the delete seqNo on a follower?

@henningandersen Two reasons that I removed it:

This should be a noop for the following engine since MSU should be at least the seqno of a delete.

We should delegate to advanceMaxSeqNoOfUpdatesOrDeletesOnPrimary not only deletes but also updates.

The downside is that we might catch bugs where MSU from the leader is not passed correctly with advanceMaxSeqNoOfUpdatesOrDeletesOnPrimary.

I am good with either keeping or removing it. What do you think?

I opt to remove it. One option to mitigate the downside is to override delete() and assert that seqNo <= MSU in FollowingEngine?

good plan. We talked about this, Nhat. After reflection, let's do it here in this PR.

I pushed 0f4492b to restore this method and add an assertion in the following engine.

dnhatn · 2019-04-27T00:19:50Z

@henningandersen @ywelsch This is ready for another round. Can you please give it go? Thank you!

ywelsch

LGTM

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

dnhatn · 2019-04-29T19:45:48Z

@henningandersen Are you good with this change?

henningandersen

LGTM

dnhatn · 2019-04-30T00:56:46Z

@henningandersen @ywelsch Thank you for your reviews.

Today we choose to initialize max_seq_no_of_updates on primaries only so we can deal with a situation where a primary is on an old node (before 6.5) which does not have MUS while replicas on new nodes (6.5+). However, this strategy is quite complex and can lead to bugs (for example #40249) since we have to assign a correct value (not too low) to MSU in all possible situations (before recovering from translog, restoring history on promotion, and handing off relocation). Fortunately, we don't have to deal with this BWC in 7.0+ since all nodes in the cluster should have MSU. This change simplifies the initialization of MSU by always assigning it a correct value in the constructor of Engine regardless of whether it's a replica or primary. Relates #33842

Today we choose to initialize max_seq_no_of_updates on primaries only so we can deal with a situation where a primary is on an old node (before 6.5) which does not have MUS while replicas on new nodes (6.5+). However, this strategy is quite complex and can lead to bugs (for example elastic#40249) since we have to assign a correct value (not too low) to MSU in all possible situations (before recovering from translog, restoring history on promotion, and handing off relocation). Fortunately, we don't have to deal with this BWC in 7.0+ since all nodes in the cluster should have MSU. This change simplifies the initialization of MSU by always assigning it a correct value in the constructor of Engine regardless of whether it's a replica or primary. Relates elastic#33842

Today we choose to initialize max_seq_no_of_updates on primaries only so we can deal with a situation where a primary is on an old node (before 6.5) which does not have MUS while replicas on new nodes (6.5+). However, this strategy is quite complex and can lead to bugs (for example #40249) since we have to assign a correct value (not too low) to MSU in all possible situations (before recovering from translog, restoring history on promotion, and handing off relocation). Fortunately, we don't have to deal with this BWC in 7.0+ since all nodes in the cluster should have MSU. This change simplifies the initialization of MSU by always assigning it a correct value in the constructor of Engine regardless of whether it's a replica or primary. Relates #33842 Port of elastic/elasticsearch#41161

Today we choose to initialize max_seq_no_of_updates on primaries only so we can deal with a situation where a primary is on an old node (before 6.5) which does not have MUS while replicas on new nodes (6.5+). However, this strategy is quite complex and can lead to bugs (for example #40249) since we have to assign a correct value (not too low) to MSU in all possible situations (before recovering from translog, restoring history on promotion, and handing off relocation). Fortunately, we don't have to deal with this BWC in 7.0+ since all nodes in the cluster should have MSU. This change simplifies the initialization of MSU by always assigning it a correct value in the constructor of Engine regardless of whether it's a replica or primary. Relates #33842 Port of elastic/elasticsearch#41161 (cherry picked from commit 62ebfb0)

Simplify initialization of max_seq_no of updates

d137d8d

dnhatn added >enhancement :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v7.2.0 labels Apr 12, 2019

dnhatn requested review from s1monw, ywelsch and henningandersen April 12, 2019 18:09

dnhatn added 4 commits April 12, 2019 14:49

remove unused imports

82c09bb

fix tests

272ae44

remove unused stuffs

1b5d839

Merge branch 'master' into init-msu

cc242c4

henningandersen reviewed Apr 14, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/engine/Engine.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/engine/Engine.java Outdated Show resolved Hide resolved

Move it to InternalEngine

1260735

dnhatn requested a review from henningandersen April 14, 2019 22:28

remove static import

69b6145

henningandersen reviewed Apr 15, 2019

View reviewed changes

dnhatn added 2 commits April 15, 2019 10:10

Merge branch 'master' into init-msu

884080a

improve read-only engine

a01cefe

dnhatn requested a review from henningandersen April 15, 2019 16:16

dnhatn added 2 commits April 26, 2019 18:56

Merge branch 'master' into init-msu

81873b5

Restore advanceMaxSeqNoOfUpdatesOrDeletesOnPrimary

0f4492b

dnhatn requested review from ywelsch and removed request for s1monw, ywelsch and henningandersen April 27, 2019 00:19

dnhatn requested review from henningandersen and ywelsch April 27, 2019 00:20

ywelsch approved these changes Apr 29, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java Outdated Show resolved Hide resolved

dnhatn added 2 commits April 29, 2019 14:57

Merge branch 'master' into init-msu

17a22af

wording

22315d6

henningandersen approved these changes Apr 29, 2019

View reviewed changes

dnhatn merged commit 73bfdc4 into elastic:master Apr 30, 2019

dnhatn deleted the init-msu branch April 30, 2019 00:57

dnhatn added the backport pending label Apr 30, 2019

dnhatn removed the backport pending label Apr 30, 2019

henningandersen mentioned this pull request Jun 7, 2019

[CI] InternalEngineTests.testLookupSeqNoByIdInLucene fails after prune ID merge policy #42979

Closed

dnhatn mentioned this pull request Jun 26, 2019

Enable indexing optimization using sequence numbers on replicas #43616

Merged

marregui mentioned this pull request Nov 29, 2019

Rebuild version map when opening internal engine crate/crate#9394

Merged

marregui mentioned this pull request Dec 2, 2019

Simplify initialization of max_seq_no of updates crate/crate#9408

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify initialization of max_seq_no of updates #41161

Simplify initialization of max_seq_no of updates #41161

dnhatn commented Apr 12, 2019 •

edited

Loading

elasticmachine commented Apr 12, 2019

henningandersen left a comment •

edited by dnhatn

Loading

dnhatn commented Apr 14, 2019

henningandersen left a comment

henningandersen Apr 15, 2019

dnhatn Apr 15, 2019

henningandersen Apr 15, 2019

ywelsch Apr 26, 2019

dnhatn Apr 27, 2019

dnhatn commented Apr 27, 2019

ywelsch left a comment

dnhatn commented Apr 29, 2019

henningandersen left a comment

dnhatn commented Apr 30, 2019

Simplify initialization of max_seq_no of updates #41161

Simplify initialization of max_seq_no of updates #41161

Conversation

dnhatn commented Apr 12, 2019 • edited Loading

elasticmachine commented Apr 12, 2019

henningandersen left a comment • edited by dnhatn Loading

Choose a reason for hiding this comment

dnhatn commented Apr 14, 2019

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen Apr 15, 2019

Choose a reason for hiding this comment

dnhatn Apr 15, 2019

Choose a reason for hiding this comment

henningandersen Apr 15, 2019

Choose a reason for hiding this comment

ywelsch Apr 26, 2019

Choose a reason for hiding this comment

dnhatn Apr 27, 2019

Choose a reason for hiding this comment

dnhatn commented Apr 27, 2019

ywelsch left a comment

Choose a reason for hiding this comment

dnhatn commented Apr 29, 2019

henningandersen left a comment

Choose a reason for hiding this comment

dnhatn commented Apr 30, 2019

dnhatn commented Apr 12, 2019 •

edited

Loading

henningandersen left a comment •

edited by dnhatn

Loading