-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[Zen2] PersistedState interface implementation #35819
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
Conversation
|
Pinging @elastic/es-distributed |
ywelsch
left a comment
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've left some comments, the main one being about initialization where the localNode object is not available yet. I'm also not sure about the class names Zen1GatewayMetaState and Zen2GatewayMetaState, but we can address that in a follow-up.
server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/Zen2GatewayMetaState.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Outdated
Show resolved
Hide resolved
DaveCTurner
left a comment
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 would really like to see an ESIntegTestCase that forms a Zen2 cluster and performs a full cluster restart here. Is that possible, or is the lack of changes to state recovery an obstruction? That kind of test would catch things like this
https://github.com/elastic/elasticsearch/pull/35819/files#r235791731 for instance.
|
@ywelsch @DaveCTurner, I've made further changes, could you please take a look?
|
ywelsch
left a comment
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've left some more smaller comments. Looking good already.
server/src/main/java/org/elasticsearch/cluster/service/ClusterApplier.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/discovery/DiscoveryModule.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/coordination/PersistedStateIT.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/discovery/zen/ZenDiscoveryUnitTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/test/discovery/TestZenDiscovery.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Outdated
Show resolved
Hide resolved
030f9ae to
5bdff70
Compare
35d3887 to
24c43fa
Compare
|
@ywelsch I'm done with the changes, ready for the next pass. |
ywelsch
left a comment
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
Today
GatewayMetaStateis capable of atomically storingMetaDatatodisk. We've also moved fields that are needed to be persisted in Zen2
from
ClusterStatetoClusterState.MetaData.CoordinationMetaData.This PR implements
PersistedStateinterface.versionandcurrentTermare persisted as a part ofMainfest.GatewayMetaStatenow have two descendantsZen1GatewayMetaStateand
Zen2GatewayMetaState(which implementsPersistedState).GatewayMetaStatenow constructspreviousClusterState(includingMetaData) andpreviousManifestinside the constructor, so that allPersistedStatemethods are usable as soon asGatewayMetaStateinstance is constructed. Also
loadMetaDatais renamed togetMetaData, because it just returnspreviousClusterState.metaData().IndexMetaDatato disk,we're comparing current
IndexMetaDataversion and receivedIndexMetaDataversion. This is not safe in Zen2 if term has changed.So
updateClusterStatenow acceptswriteWithoutComparingVersionmethod parameter. When it's set to true, we always write
IndexMetaDatato disk.GatewayMetaStateTestsare coveredby
Zen2GatewayMetaStateTests.This PR does not inject
Zen2GatewayMetaStatein place ofZen1GatewayMetaStateand in place ofInMemoryPersistedState, thisis a work to be done in a follow-up PR.