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.

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

Add min. read-only index version compatible to DiscoveryNode #118744

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Dec 16, 2024

#118443 added a new index version for indices that can be opened in read-only mode by Lucene. This change adds this information to the discovery node's VersionInformation and the transport serialization logic.

In a short future we'd like to use this information in methods like IndexMetadataVerifier#checkSupportedVersion and NodeJoineExecutor to allow opening indices in N-2 versions as read-only indices on ES V9.

@tlrx tlrx added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v9.0.0 v8.18.0 labels Dec 16, 2024
@tlrx tlrx marked this pull request as ready for review December 16, 2024 13:56
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Dec 16, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

public void testDiscoveryNodeMinReadOnlyVersionSerialization() throws Exception {
var node = DiscoveryNodeUtils.create(
"_id",
new TransportAddress(
Copy link
Member

Choose a reason for hiding this comment

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

Why not just ESTestCase.buildNewFakeTransportAddress()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No special reason, thanks for catching this. I pushed d23319b

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

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

Change looks ok in principle

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

IndexVersion minIndexVersion = IndexVersion.readVersion(in);
IndexVersion minReadOnlyIndexVersion;
if (in.getTransportVersion().onOrAfter(NODE_VERSION_INFORMATION_WITH_MIN_READ_ONLY_INDEX_VERSION)) {
minReadOnlyIndexVersion = IndexVersion.readVersion(in);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is good, but perhaps you can confirm my thinking here. I originally worried a little about it persisting due to residing in cluster state such that it would depend on the master's version.

It should work though since we require a full upgrade to 8.18 before upgrading to 9.0 (at least in a rolling fashion). Hence on 8.18, any master here would then see a new 9.0 node, hence it would have the N-2 min-read-only-version.

All good I think, just overcomplicating it (I think).

Copy link
Member Author

@tlrx tlrx Dec 17, 2024

Choose a reason for hiding this comment

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

I think it is OK too, yet it is not an area I'm super confortable with.

My understanding is that a cluster on 8.17 with indices created in 7.x should reject join requests from 9.0 nodes since the node will only communicate a min. index compatible version on 8.

Once this PR is merged in main and backported to 8.18, a master on 8.18 will receive join requests from 9.0 nodes with both the N-1 and N-2 information but should still reject the node if 7.x indices exist in the cluster since 9.0 is not able to read andwrite such indices (I expect to change this soon for read-only searchable snapshot indices in #118785).

@tlrx tlrx added the auto-backport Automatically create backport pull requests when merged label Dec 17, 2024
@tlrx tlrx merged commit f3a1664 into elastic:main Dec 17, 2024
15 of 16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118744

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 17, 2024
…lastic#118744)

In a short future we'd like to use this information in methods like IndexMetadataVerifier#checkSupportedVersion and NodeJoineExecutor to allow opening indices in N-2 versions as read-only indices on ES V9.
elasticsearchmachine pushed a commit that referenced this pull request Dec 17, 2024
…118744) (#118884)

* [8.x] Add min. read-only index version compatible to DiscoveryNode (#118744)

In a short future we'd like to use this information in methods like IndexMetadataVerifier#checkSupportedVersion and NodeJoineExecutor to allow opening indices in N-2 versions as read-only indices on ES V9.

* MINIMUM_READONLY_COMPATIBLE
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Dec 18, 2024
…#118744)

elastic#118443 added a new index version for indices that can be opened in read-only mode by Lucene. This change adds this information to the discovery node's VersionInformation and the transport serialization logic.

In a short future we'd like to use this information in methods like IndexMetadataVerifier#checkSupportedVersion and NodeJoineExecutor to allow opening indices in N-2 versions as read-only indices on ES V9.
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 19, 2024
We prefer to remove this information from the API since they
are not useful externally, impact the search shard API and
may be removed later (which would be a breaking change).

Follow-up elastic#118744
elasticsearchmachine pushed a commit that referenced this pull request Dec 19, 2024
We prefer to remove this information from the API since they are not
useful externally, impact the search shard API and may be removed later
(which would be a breaking change).

Follow-up #118744
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 19, 2024
We prefer to remove this information from the API since they are not
useful externally, impact the search shard API and may be removed later
(which would be a breaking change).

Follow-up elastic#118744
elasticsearchmachine pushed a commit that referenced this pull request Dec 19, 2024
…#119114)

* Remove min_read_only_index_version from XContent node (#119083)

We prefer to remove this information from the API since they are not
useful externally, impact the search shard API and may be removed later
(which would be a breaking change).

Follow-up #118744

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants