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.

Sign up for GitHub

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

Jump to bottom

Introduce constants for special ReservedClusterStateMetadata versions #107995

Merged
merged 3 commits into from
May 7, 2024
Merged

Introduce constants for special ReservedClusterStateMetadata versions #107995

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
823682b
Introduce constants for special ReservedClusterStateMetadata versions
mosche Apr 29, 2024
247c9b9
feedback
mosche May 7, 2024
2776107
Merge branch 'main' into reserved_cluster_state_versions
elasticmachine May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/cluster/metadata/ReservedStateMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public record ReservedStateMetadata(
) implements SimpleDiffable<ReservedStateMetadata>, ToXContentFragment {

public static final Long NO_VERSION = Long.MIN_VALUE; // use min long as sentinel for uninitialized version
public static final Long EMPTY_VERSION = -1L; // use -1 as sentinel for empty metadata
public static final Long RESTORED_VERSION = 0L; // use 0 as sentinel for metadata restored from snapshot

private static final ParseField VERSION = new ParseField("version");
private static final ParseField HANDLERS = new ParseField("handlers");
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ protected boolean shouldRefreshFileState(ClusterState clusterState) {
// We check if the version was reset to 0, and force an update if a file exists. This can happen in situations
// like snapshot restores.
ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(NAMESPACE);
return fileSettingsMetadata != null && fileSettingsMetadata.version() == 0L;
return fileSettingsMetadata != null && fileSettingsMetadata.version().equals(ReservedStateMetadata.RESTORED_VERSION);
}

/**
Expand Down
7 changes: 4 additions & 3 deletions ...er/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.util.stream.Collectors;

import static org.elasticsearch.ExceptionsHelper.stackTrace;
import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.EMPTY_VERSION;
import static org.elasticsearch.core.Strings.format;
import static org.elasticsearch.reservedstate.service.ReservedStateErrorTask.checkErrorVersion;
import static org.elasticsearch.reservedstate.service.ReservedStateErrorTask.isNewError;
Expand Down Expand Up @@ -112,7 +113,7 @@ ReservedStateChunk parse(String namespace, XContentParser parser) {
try {
return stateChunkParser.apply(parser, null);
} catch (Exception e) {
ErrorState errorState = new ErrorState(namespace, -1L, e, ReservedStateErrorMetadata.ErrorKind.PARSING);
ErrorState errorState = new ErrorState(namespace, EMPTY_VERSION, e, ReservedStateErrorMetadata.ErrorKind.PARSING);
updateErrorState(errorState);
logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState);

Expand All @@ -134,7 +135,7 @@ public void process(String namespace, XContentParser parser, Consumer<Exception>
try {
stateChunk = parse(namespace, parser);
} catch (Exception e) {
ErrorState errorState = new ErrorState(namespace, -1L, e, ReservedStateErrorMetadata.ErrorKind.PARSING);
ErrorState errorState = new ErrorState(namespace, EMPTY_VERSION, e, ReservedStateErrorMetadata.ErrorKind.PARSING);
updateErrorState(errorState);
logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState);

Expand All @@ -148,7 +149,7 @@ public void process(String namespace, XContentParser parser, Consumer<Exception>
}

public void initEmpty(String namespace, ActionListener<ActionResponse.Empty> listener) {
var missingVersion = new ReservedStateVersion(-1L, Version.CURRENT);
var missingVersion = new ReservedStateVersion(EMPTY_VERSION, Version.CURRENT);
var emptyState = new ReservedStateChunk(Map.of(), missingVersion);
updateTaskQueue.submitTask(
"empty initial cluster state [" + namespace + "]",
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateErrorTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.cluster.metadata.ReservedStateErrorMetadata;
import org.elasticsearch.cluster.metadata.ReservedStateMetadata;

import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.RESTORED_VERSION;
import static org.elasticsearch.core.Strings.format;

/**
Expand Down Expand Up @@ -50,7 +51,7 @@ ActionListener<ActionResponse.Empty> listener() {
static boolean isNewError(ReservedStateMetadata existingMetadata, Long newStateVersion) {
return (existingMetadata == null
|| existingMetadata.errorMetadata() == null
|| newStateVersion <= 0 // version will be -1 when we can't even parse the file, it might be 0 on snapshot restore
|| newStateVersion <= RESTORED_VERSION // Long.MIN_VALUE (no version) when we can't parse the file, 0 on snapshot restore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, if there are no other cases, if
newStateVersion == RESTORED_VERSION || newStateVersion == EMPTY_VERSION is more clear (you could get rid of the comment, which is usually a good indication)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this as well, and agree it would add clarity. At the same time I'm worried this would easily break things if more magic version ids are introduced. I'll think a bit more about it....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enforce the version for error metadata is > 0 when explicitly initialized (ie with a real error)?

Copy link
Contributor Author

@mosche mosche May 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjernst I had another look at this, I think it's tricky to make any changes on error metadata version as it's coupled to the reserved state version. If a previous reserved state version exists, that one will be the version of the error. However, that can be any of the special versions <= 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldematte changed as suggested

|| existingMetadata.errorMetadata().version() < newStateVersion);
}

Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,12 @@ static boolean checkMetadataVersion(
}

// Version -1 is special, it means "empty"
if (reservedStateVersion.version() == -1L) {
if (reservedStateVersion.version().equals(ReservedStateMetadata.EMPTY_VERSION)) {
return true;
}

// Version 0 is special, snapshot restores will reset to 0.
if (reservedStateVersion.version() <= 0L) {
if (reservedStateVersion.version() <= ReservedStateMetadata.RESTORED_VERSION) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I reverted this change to <= 0L, this better captures the intention of this: check that the new version is a regular positive one

logger.warn(
() -> format(
"Not updating reserved cluster state for namespace [%s], because version [%s] is less or equal to 0",
Expand Down
3 changes: 2 additions & 1 deletion server/src/test/java/org/elasticsearch/readiness/ReadinessServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import java.util.Set;

import static org.elasticsearch.cluster.metadata.ReservedStateErrorMetadata.ErrorKind.TRANSIENT;
import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.EMPTY_VERSION;

public class ReadinessServiceTests extends ESTestCase implements ReadinessClientProbe {
private ClusterService clusterService;
Expand All @@ -59,7 +60,7 @@ public class ReadinessServiceTests extends ESTestCase implements ReadinessClient

private static Metadata emptyReservedStateMetadata;
static {
var fileSettingsState = new ReservedStateMetadata.Builder(FileSettingsService.NAMESPACE).version(-1L);
var fileSettingsState = new ReservedStateMetadata.Builder(FileSettingsService.NAMESPACE).version(EMPTY_VERSION);
emptyReservedStateMetadata = new Metadata.Builder().put(fileSettingsState.build()).build();
}

Expand Down