-
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
[Zen2] PersistedState interface implementation #35819
Conversation
Pinging @elastic/es-distributed |
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
Outdated
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
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/gateway/Zen2GatewayMetaStateTests.java
Outdated
Show resolved
Hide resolved
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?
|
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
Outdated
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. |
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
GatewayMetaState
is capable of atomically storingMetaData
todisk. We've also moved fields that are needed to be persisted in Zen2
from
ClusterState
toClusterState.MetaData.CoordinationMetaData
.This PR implements
PersistedState
interface.version
andcurrentTerm
are persisted as a part ofMainfest
.GatewayMetaState
now have two descendantsZen1GatewayMetaState
and
Zen2GatewayMetaState
(which implementsPersistedState
).GatewayMetaState
now constructspreviousClusterState
(includingMetaData
) andpreviousManifest
inside the constructor, so that allPersistedState
methods are usable as soon asGatewayMetaState
instance is constructed. Also
loadMetaData
is renamed togetMetaData
, because it just returnspreviousClusterState.metaData()
.IndexMetaData
to disk,we're comparing current
IndexMetaData
version and receivedIndexMetaData
version. This is not safe in Zen2 if term has changed.So
updateClusterState
now acceptswriteWithoutComparingVersion
method parameter. When it's set to true, we always write
IndexMetaData
to disk.GatewayMetaStateTests
are coveredby
Zen2GatewayMetaStateTests
.This PR does not inject
Zen2GatewayMetaState
in place ofZen1GatewayMetaState
and in place ofInMemoryPersistedState
, thisis a work to be done in a follow-up PR.