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

Always re-run Feature migrations which have encountered errors #83918

Merged
merged 10 commits into from
Feb 17, 2022

Conversation

gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Feb 15, 2022

This PR addressed the behavior described in #83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features
requiring migration.

This PR also adds a test which artificially replicates #83917.

Fixes #83917.

@gwbrown gwbrown requested review from rjernst and grcevski February 15, 2022 01:53
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Feb 15, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@gwbrown gwbrown changed the title Re-run errors Always re-run Feature migrations which have encountered errors Feb 15, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @gwbrown, I've created a changelog YAML for you.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple comments on the test.


@Override
public void onFailure(Exception e) {
clusterStateUpdated.set(false);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be captured and cause the assertBusy to fail with an assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good point.

PostFeatureUpgradeRequest migrationRequest = new PostFeatureUpgradeRequest();
PostFeatureUpgradeResponse migrationResponse = client().execute(PostFeatureUpgradeAction.INSTANCE, migrationRequest).get();
// Make sure we actually started the migration
final Set<String> migratingFeatures = migrationResponse.getFeatures()
Copy link
Member

Choose a reason for hiding this comment

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

Converting to a set isn't anymore efficient than just looking through the list. Is there som other reason we need an intermediate set to assert the feature was processed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a copy/paste from another test, you're right there's not much point to converting to a set here. I'll simplify.

@gwbrown gwbrown requested a review from rjernst February 17, 2022 00:46
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@gwbrown gwbrown merged commit 4bc6a12 into elastic:master Feb 17, 2022
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Feb 17, 2022
…ic#83918)

This PR addressed the behavior described in elastic#83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates elastic#83917.
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Feb 17, 2022
…ic#83918)

This PR addressed the behavior described in elastic#83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates elastic#83917.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.17
8.0
8.1

elasticsearchmachine pushed a commit that referenced this pull request Feb 17, 2022
… (#84139)

This PR addressed the behavior described in #83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates #83917.
elasticsearchmachine pushed a commit that referenced this pull request Feb 17, 2022
…#83918) (#84137)

* Always re-run Feature migrations which have encountered errors (#83918)

This PR addressed the behavior described in #83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates #83917.

* Adjust for method signature differences in this branch
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 18, 2022
* upstream/master: (167 commits)
  Mute FrozenSearchableSnapshotsIntegTests#testCreateAndRestorePartialSearchableSnapshot
  Mute LdapSessionFactoryTests#testSslTrustIsReloaded
  Fix spotless violation from last commit
  Mute GeoGridTilerTestCase#testGeoGridSetValuesBoundingBoxes_UnboundedGeoShapeCellValues
  Small formatting clean up (elastic#84144)
  Always re-run Feature migrations which have encountered errors (elastic#83918)
  [DOCS] Clarify `orientation` usage for WKT and GeoJSON polygons (elastic#84025)
  Group field caps response by index mapping hash (elastic#83494)
  Shrink join queries in slow log (elastic#83914)
  TSDB: Reject the nested object fields that are configured time_series_dimension (elastic#83920)
  [DOCS] Remove note about partial response from Bulk API docs (elastic#84053)
  Allow regular data streams to be migrated to tsdb data streams. (elastic#83843)
  [DOCS] Fix `ignore_unavailable` parameter definition (elastic#84071)
  Make Metadata extend AbstractCollection (elastic#83791)
  Add API specs for OpenID Connect APIs
  Revert "Clean up for superuser role name references (elastic#83627)" (elastic#84096)
  Update Lucene analysis base url (elastic#84094)
  Avoid null threadContext in ResultDeduplicator (elastic#84093)
  Use static empty store files metadata (elastic#84034)
  Preserve context in snapshotDeletionListeners (elastic#84089)
  ...

# Conflicts:
#	x-pack/plugin/rollup/build.gradle
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Feb 22, 2022
…ic#83918)

This PR addressed the behavior described in elastic#83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates elastic#83917.
elasticsearchmachine pushed a commit that referenced this pull request Feb 22, 2022
…83918) (#84138)

* Always re-run Feature migrations which have encountered errors (#83918)

This PR addressed the behavior described in #83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates #83917.

* Resolve compilation failures for 8.0 branch
probakowski pushed a commit to probakowski/elasticsearch that referenced this pull request Feb 23, 2022
…ic#83918)

This PR addressed the behavior described in elastic#83917, in which Feature migrations
which have encountered errors are not re-run in some cases. As of this PR, Features
which have encountered errors during migration are treated the same as Features 
requiring migration.

This PR also adds a test which artificially replicates elastic#83917.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v7.17.1 v8.0.1 v8.1.1 v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature migration refuses to run if all features requiring upgrade have encountered errors
4 participants