[TEST] More MetadataStateFormat tests by grcevski · Pull Request #78577 · elastic/elasticsearch

grcevski · 2021-10-01T21:23:06Z

Add test about partial failure on multi-paths. Having stale metadata is expected and OK with cleanUp set to false
Add test for deleteMetaState

Relates to #78475

Add test about partial failure on multi-paths.

elasticmachine · 2021-10-06T18:47:19Z

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

rjernst

The new tests look good to me. @henningandersen do you have any thoughts?

rjernst · 2021-10-06T19:34:56Z

The CI failure is #78783, syncing with master should mute it.

grcevski · 2021-10-06T20:16:16Z

@elasticmachine run elasticsearch-ci/part-2

henningandersen

Thanks for improving our test coverage, Nikola. I left some smaller comments, otherwise this looks good.

henningandersen · 2021-10-07T11:04:51Z

server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java

     * @throws IOException if IOException occurs
     */
-    private long findMaxGenerationId(final String prefix, Path... locations) throws IOException {
+    protected long findMaxGenerationId(final String prefix, Path... locations) throws IOException {


Can we make the visibility of this package private instead?

henningandersen · 2021-10-07T11:04:57Z

server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java

    }

-    private List<Path> findStateFilesByGeneration(final long generation, Path... locations) {
+    protected List<Path> findStateFilesByGeneration(final long generation, Path... locations) {


Can we make the visibility of this package private instead?

henningandersen · 2021-10-07T11:12:09Z

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

        }

+        private void throwDirectoryExceptionCheckPaths(Path dir) throws MockDirectoryWrapper.FakeIOException {
+            if (failurePaths != null && failurePaths.length > 0) {


nit: no need to be defensive here, I would prefer to have just null signal no paths.

henningandersen · 2021-10-07T11:22:27Z

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

+        assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[1]));
+        assertEquals(genId, format.findMaxGenerationId("foo-", badPath));
+        assertEquals(genId, firstPathId-1);
+    }


I think we can also verify that we can find max genId and read state when supplying all paths (and get the new state out)?

henningandersen · 2021-10-07T11:28:31Z

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

+        for (Path path : paths) {
+            assertEquals(false, Files.exists(path.resolve(MetadataStateFormat.STATE_DIR_NAME)));
+        }
+    }


Can we also verify that format.loadLatestStage(..., paths) return null and that findMaxGenerationId return -1?

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

henningandersen · 2021-10-07T11:37:07Z

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

+        corruptFile(stateFiles.get(1), logger);
+
+        // Ensure we find the corrupted metadata without the leader first state path
+        expectThrows(ElasticsearchException.class, () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths));


I am not sure I follow this. I would think the state file in path 1 should still be available? Maybe add a comment plus verification of main part of exception message to ensure we hit the right exception?

Sounds good! I'll clarify this bit. Since I cleaned up the old files, but caused the middle path to fail, there's only one stale state file left that is now corrupted and this is ensuring we detect the corrupted state.

…rmatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

…sticsearch into tests/metadata_state_format

grcevski · 2021-10-07T15:25:57Z

Thanks for improving our test coverage, Nikola. I left some smaller comments, otherwise this looks good.

Thanks for reviewing this @henningandersen, great suggestions! I made the changes to address the review comments.

henningandersen

LGTM.

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java

…rmatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

- Add test about partial failure on multi-paths - Add test for deleteMetaState Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

Backport of: #78577 - Add test about partial failure on multi-paths - Add test for deleteMetaState Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

* master: Fix DataTierTests package and add a validation test (elastic#78880) Fix split package org.elasticsearch.common.xcontent (elastic#78831) Store DataTier Preference directly on IndexMetadata (elastic#78668) [DOCS] Fixes typo in calendar API example (elastic#78867) Improve Node Shutdown Observability (elastic#78727) Convert encrypted snapshot license object to LicensedFeature (elastic#78731) Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801) Fix incorrect generic type in PolicyStepsRegistry (elastic#78628) [DOCS] Fixes ML get calendars API (elastic#78808) Implement GET API for System Feature Upgrades (elastic#78642) [TEST] More MetadataStateFormat tests (elastic#78577) Add support for rest compatibility headers to the HLRC (elastic#78490) Un-ignoring tests after backporting fix (elastic#78830) Add node REPLACE shutdown implementation (elastic#76247) Wrap VersionPropertiesLoader in a BuildService to decouple build logic projects (elastic#78704) Adjust /_cat/templates not to request all metadata (elastic#78829) [DOCS] Fixes ML get scheduled events API (elastic#78809) Enable exit on out of memory error (elastic#71542) # Conflicts: # server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

@OverRide

* upstream/master: (250 commits) [Transform] HLRC cleanups (elastic#78909) [ML] Make ML indices hidden when the node becomes master (elastic#77416) Introduce a Few Settings Singleton Instances (elastic#78897) Simplify TestCluster extraJar configuration (elastic#78837) Add @OverRide annotations to methods in EnrichPlugin class (elastic#76873) Add v7 restCompat for invalidating API key with the id field (elastic#78664) EQL: Refine repeatable queries (elastic#78895) Fix DataTierTests package and add a validation test (elastic#78880) Fix split package org.elasticsearch.common.xcontent (elastic#78831) Store DataTier Preference directly on IndexMetadata (elastic#78668) [DOCS] Fixes typo in calendar API example (elastic#78867) Improve Node Shutdown Observability (elastic#78727) Convert encrypted snapshot license object to LicensedFeature (elastic#78731) Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801) Fix incorrect generic type in PolicyStepsRegistry (elastic#78628) [DOCS] Fixes ML get calendars API (elastic#78808) Implement GET API for System Feature Upgrades (elastic#78642) [TEST] More MetadataStateFormat tests (elastic#78577) Add support for rest compatibility headers to the HLRC (elastic#78490) Un-ignoring tests after backporting fix (elastic#78830) ... # Conflicts: # server/src/main/java/org/elasticsearch/ingest/IngestService.java # server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java

[TEST] More MetadataStateFormat tests

94b4b51

Add test about partial failure on multi-paths.

grcevski marked this pull request as draft October 1, 2021 21:23

elasticsearchmachine added the v8.0.0 label Oct 1, 2021

Nikola Grcevski added 6 commits October 1, 2021 19:14

Fix style.

b1799df

Merge branch 'master' into tests/metadata_state_format

6027cea

Improve tests, add delete tests.

6da40e2

Merge branch 'master' into tests/metadata_state_format

138d929

Update test based on expected behaviour.

1a1e801

Merge branch 'master' into tests/metadata_state_format

877c998

grcevski added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team labels Oct 6, 2021

grcevski requested a review from rjernst October 6, 2021 18:45

grcevski marked this pull request as ready for review October 6, 2021 18:47

rjernst approved these changes Oct 6, 2021

View reviewed changes

Merge branch 'master' into tests/metadata_state_format

0122f8d

henningandersen reviewed Oct 7, 2021

View reviewed changes

Nikola Grcevski and others added 6 commits October 7, 2021 09:58

Implement PR suggestions.

84ccc3b

Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFo…

1a8e8b1

…rmatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFo…

a3e1bb7

…rmatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

Merge branch 'tests/metadata_state_format' of github.com:grcevski/ela…

e1b415e

…sticsearch into tests/metadata_state_format

More PR suggestions implementation.

b3eeb4e

Merge branch 'master' into tests/metadata_state_format

e1a2714

Fix import.

9467926

henningandersen self-requested a review October 7, 2021 16:26

henningandersen approved these changes Oct 7, 2021

View reviewed changes

server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java Outdated Show resolved Hide resolved

grcevski and others added 2 commits October 7, 2021 12:51

Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFo…

2b37ee5

…rmatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

Fix line width.

0b4b055

grcevski merged commit 631e066 into elastic:master Oct 7, 2021

grcevski deleted the tests/metadata_state_format branch October 7, 2021 20:48

grcevski mentioned this pull request Oct 7, 2021

[7.x] [TEST] More MetadataStateFormat tests (#78577) #78859

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Conversation

grcevski commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 6, 2021

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst commented Oct 6, 2021

Uh oh!

grcevski commented Oct 6, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henningandersen Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

grcevski Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

grcevski commented Oct 7, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

grcevski commented Oct 1, 2021 •

edited

Loading