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

Act on version 8 related asserts #38556

Closed
alpar-t opened this issue Feb 7, 2019 · 5 comments
Closed

Act on version 8 related asserts #38556

alpar-t opened this issue Feb 7, 2019 · 5 comments
Assignees
Labels
:Core/Infra/Core Core issues without another label v8.0.0-alpha1

Comments

@alpar-t
Copy link
Contributor

alpar-t commented Feb 7, 2019

We have a number of assertions for checking that we did not pump to version 8 yet.
I am in the process of muting them thus this issue to track them.
This is necessary so we can merge #38514 and get builds going.

  • ResyncReplicationRequest, MetaDataCreateIndexService, RemoteClusterAware and RemoteClusterAware added by @jasontedor
  • MetaStateService added by @andrershov
@alpar-t alpar-t added the v8.0.0 label Feb 7, 2019
alpar-t added a commit to jasontedor/elasticsearch that referenced this issue Feb 7, 2019
@colings86 colings86 added the :Core/Infra/Core Core issues without another label label Feb 7, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member

@DaveCTurner Can you clean up the last remaining version 8 assertion in the code base? It's currently commented out:

if (globalMetaData != null) {
metaDataBuilder = MetaData.builder(globalMetaData);
indexGraveyard = globalMetaData.custom(IndexGraveyard.TYPE);
// TODO https://github.com/elastic/elasticsearch/issues/38556
// assert Version.CURRENT.major < 8 : "failed to find manifest file, which is mandatory staring with Elasticsearch version 8.0";
} else {

so there's some dead code there that theoretically can be cleaned up and removed.

@jasontedor
Copy link
Member

There's an assertion in this block of code too:

for (String indexFolderName : nodeEnv.availableIndexFolders()) {
Tuple<IndexMetaData, Long> indexMetaDataAndGeneration =
INDEX_META_DATA_FORMAT.loadLatestStateWithGeneration(logger, namedXContentRegistry,
nodeEnv.resolveIndexFolder(indexFolderName));
// TODO https://github.com/elastic/elasticsearch/issues/38556
// assert Version.CURRENT.major < 8 : "failed to find manifest file, which is mandatory staring with Elasticsearch version 8.0";
IndexMetaData indexMetaData = indexMetaDataAndGeneration.v1();
long generation = indexMetaDataAndGeneration.v2();
if (indexMetaData != null) {
if (indexGraveyard.containsIndex(indexMetaData.getIndex())) {
logger.debug("[{}] found metadata for deleted index [{}]", indexFolderName, indexMetaData.getIndex());
// this index folder is cleared up when state is recovered
} else {
indices.put(indexMetaData.getIndex(), generation);
metaDataBuilder.put(indexMetaData, false);
}
} else {
logger.debug("[{}] failed to find metadata for existing index location", indexFolderName);
}
}
Manifest manifest = Manifest.unknownCurrentTermAndVersion(globalStateGeneration, indices);
return new Tuple<>(manifest, metaDataBuilder.build());
}

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Feb 17, 2020
7.x nodes permit the on-disk cluster metadata to omit the manifest file in
order to support upgrades from 6.x. We are similarly lenient in `master`, i.e.
8.x, but there is no need to be since we must be upgrading from a 7.x node
which ensures that the manifest file is written.

This commit removes the lenient loading of a manifest-free cluster metadata
from `master`.

Relates elastic#38556
@DaveCTurner
Copy link
Contributor

Sure, I opened #52412.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Feb 21, 2020
7.x nodes permit the on-disk cluster metadata to omit the manifest file in
order to support upgrades from 6.x. We are similarly lenient in `master`, i.e.
8.x, but there is no need to be since we must be upgrading from a 7.x node
which ensures that the manifest file is written.

We prefer to keep this lenience (see elastic#52412) so this commit removes the
commented-out indications that it should be removed. The new metadata format
introduced in elastic#50907 means that this whole subsystem will be removed in v9
anyway.

Relates elastic#38556
DaveCTurner added a commit that referenced this issue Feb 21, 2020
7.x nodes permit the on-disk cluster metadata to omit the manifest file in
order to support upgrades from 6.x. We are similarly lenient in `master`, i.e.
8.x, but there is no need to be since we must be upgrading from a 7.x node
which ensures that the manifest file is written.

We prefer to keep this lenience (see #52412) so this commit removes the
commented-out indications that it should be removed. The new metadata format
introduced in #50907 means that this whole subsystem will be removed in v9
anyway.

Relates #38556
@DaveCTurner
Copy link
Contributor

@jasontedor I merged #52646. Since you say that this was the last remaining assertions here, should we close this issue now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label v8.0.0-alpha1
Projects
None yet
Development

No branches or pull requests

6 participants