Remove test usages of `getDefaultBackingIndexName` in CCR tests by nielsbauman · Pull Request #127693 · elastic/elasticsearch

nielsbauman · 2025-05-05T05:43:48Z

We replace usages of time sensitive
DataStream#getDefaultBackingIndexName with the retrieval of the name via an API call. The problem with using the time sensitive method is that we can have test failures around midnight.

Relates #123376

We replace usages of time sensitive `DataStream#getDefaultBackingIndexName` with the retrieval of the name via an API call. The problem with using the time sensitive method is that we can have test failures around midnight. Relates elastic#123376

nielsbauman · 2025-05-05T05:44:30Z

...ck/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AbstractCCRRestTestCase.java

-    protected static boolean indexExists(String index) throws IOException {
-        Response response = adminClient().performRequest(new Request("HEAD", "/" + index));
-        return RestStatus.OK.getStatus() == response.getStatusLine().getStatusCode();
-    }


This method was already defined (with a better implementation) in ESRestTestCase.

nielsbauman · 2025-05-05T05:45:17Z

...ck/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AbstractCCRRestTestCase.java

-    /**
-     * Fix point in time when data stream backing index is first time queried.
-     * This is required to avoid failures when running test at midnight.
-     * (index is created for day0, but assertions are executed for day1 assuming different time based index name that does not exist)
-     */
-    private final LazyInitializable<Long, RuntimeException> time = new LazyInitializable<>(System::currentTimeMillis);


The idea here is nice, but it doesn't completely avoid any issues, as the cluster(s) don't use this time supplier.

nielsbauman · 2025-05-05T05:47:46Z

x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java

-            cleanUpFollower(
-                List.of(backingIndexName(dataStreamName, 1), backingIndexName(dataStreamName, 2), backingIndexName(dataStreamName, 3)),
-                List.of(dataStreamName),
-                List.of(autoFollowPatternName)
-            );
-            cleanUpLeader(
-                List.of(backingIndexName(dataStreamName, 1), backingIndexName(dataStreamName, 2), backingIndexName(dataStreamName, 3)),
-                List.of(dataStreamName),
-                List.of()
-            );
+            cleanUpFollower(List.of(), List.of(dataStreamName), List.of(autoFollowPatternName));
+            cleanUpLeader(List.of(), List.of(dataStreamName), List.of());


There are a few more places like this. When a data stream is deleted, all its backing indices are deleted as well, of course. Theoretically it's possible that there are backing indices that have a name that matches a data stream name (e.g. manually created or taken out of an existing data stream) but are not in the data stream, but that doesn't seem to happen in these tests, so we don't need to explicitly delete the backing indices.

elasticsearchmachine · 2025-05-05T05:49:20Z

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

elasticsearchmachine · 2025-05-05T05:49:20Z

Pinging @elastic/es-data-management (Team:Data Management)

nielsbauman · 2025-05-05T05:49:59Z

I added the CCR label as well, as all of these changes are in CCR tests, so I'd like to ask if someone from the distributed team could have a quick scan as well.

Copilot

Pull Request Overview

This PR replaces the time‐sensitive usage of DataStream#getDefaultBackingIndexName in CCR tests with API calls that retrieve backing index information via integer generations. The changes include replacing string-based backing index name retrieval with integer parameters in verifyDataStream calls, updating index existence checks to use adminClient(), and slimming down cleanUp methods.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java	Updated index existence checks to use adminClient().
x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/FollowIndexIT.java	Replaced use of getDefaultBackingIndexName with integer-based verifyDataStream calls.
x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/DowngradeLicenseFollowIndexIT.java	Updated index existence call with adminClient().
x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java	Revised verifyDataStream and cleanUp calls; introduced backingIndexNames variable usage.
x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AbstractCCRRestTestCase.java	Adjusted verifyDataStream implementation to use int... expectedBackingIndices.
test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java	Modified indexExists and getDataStreamBackingIndexNames to work with RestClient parameter.

Comments suppressed due to low confidence (1)

x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java:517

[nitpick] Consider using a consistent method to access the first element of backingIndexNames (for example, using get(0) if backingIndexNames is a standard List) to improve clarity and consistency with other element accesses.

followIndex(backingIndexNames.getFirst(), backingIndexNames.getFirst());

lukewhiting

It's a tidy PR in general but it does fix the issue but I can't see a replacement of the functionality... Is it ok that we no longer verify the backing index names now in these tests?

nielsbauman · 2025-05-06T11:20:21Z

@lukewhiting

Is it ok that we no longer verify the backing index names now in these tests?

That's a good question. We do still verify the backing index names here:

elasticsearch/x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AbstractCCRRestTestCase.java

Line 362 in 4256fe4

    
           assertThat(actualBackingIndex, DataStreamTestHelper.backingIndexEqualTo(name, expectedBackingIndexGeneration));

DataStreamTestHelper.backingIndexEqualTo does exactly what we want: verify the backing index name but ignore the date in the index name (i.e. it checks data stream name and generation).

Or were you referring to something else?

lukewhiting · 2025-05-06T12:28:55Z

@lukewhiting

Is it ok that we no longer verify the backing index names now in these tests?

That's a good question. We do still verify the backing index names here:

elasticsearch/x-pack/plugin/ccr/src/javaRestTest/java/org/elasticsearch/xpack/ccr/AbstractCCRRestTestCase.java

Line 362 in 4256fe4

assertThat(actualBackingIndex, DataStreamTestHelper.backingIndexEqualTo(name, expectedBackingIndexGeneration));

DataStreamTestHelper.backingIndexEqualTo does exactly what we want: verify the backing index name but ignore the date in the index name (i.e. it checks data stream name and generation).

Or were you referring to something else?

Nope that was what I was worried at but you're right, backingIndexEqualTo provides equivalent functionality :-) In that case 👍🏻 LGTM

nielsbauman · 2025-05-06T12:31:45Z

I'm going to go ahead and merge this. The changes are mostly related to data stream retrieval, so I feel confident enough to merge this already without an extra pair of eyes. It's just test logic anyway, so the impact is minimal if I/we did miss anything.

…tic#127693) We replace usages of time sensitive `DataStream#getDefaultBackingIndexName` with the retrieval of the name via an API call. The problem with using the time sensitive method is that we can have test failures around midnight. Relates elastic#123376

elasticsearchmachine added v9.1.0 needs:triage Requires assignment of a team area label labels May 5, 2025

nielsbauman commented May 5, 2025

View reviewed changes

lukewhiting requested a review from Copilot May 6, 2025 09:19

Copilot AI reviewed May 6, 2025

View reviewed changes

lukewhiting reviewed May 6, 2025

View reviewed changes

nielsbauman mentioned this pull request May 6, 2025

Remove test usages of DataStream#getDefaultBackingIndexName #123376

Closed

14 tasks

lukewhiting approved these changes May 6, 2025

View reviewed changes

Merge branch 'main' into refactor-ccr-ds-tests

6554585

nielsbauman enabled auto-merge (squash) May 6, 2025 12:30

nielsbauman merged commit a7f09b1 into elastic:main May 6, 2025
16 of 17 checks passed

nielsbauman deleted the refactor-ccr-ds-tests branch May 6, 2025 14:52

inespot mentioned this pull request Apr 8, 2026

[CI] FollowIndexSecurityIT testCleanShardFollowTaskAfterDeleteFollower failing #120339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove test usages of `getDefaultBackingIndexName` in CCR tests#127693

Remove test usages of `getDefaultBackingIndexName` in CCR tests#127693
nielsbauman merged 2 commits intoelastic:mainfrom
nielsbauman:refactor-ccr-ds-tests

nielsbauman commented May 5, 2025

Uh oh!

nielsbauman May 5, 2025

Uh oh!

nielsbauman May 5, 2025

Uh oh!

nielsbauman May 5, 2025

Uh oh!

elasticsearchmachine commented May 5, 2025

Uh oh!

elasticsearchmachine commented May 5, 2025

Uh oh!

nielsbauman commented May 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

lukewhiting left a comment •

edited

Loading

Uh oh!

nielsbauman commented May 6, 2025

Uh oh!

lukewhiting commented May 6, 2025 •

edited

Loading

Uh oh!

nielsbauman commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nielsbauman commented May 5, 2025

Uh oh!

nielsbauman May 5, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman May 5, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman May 5, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented May 5, 2025

Uh oh!

elasticsearchmachine commented May 5, 2025

Uh oh!

nielsbauman commented May 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

lukewhiting left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsbauman commented May 6, 2025

Uh oh!

lukewhiting commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nielsbauman commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukewhiting left a comment •

edited

Loading

lukewhiting commented May 6, 2025 •

edited

Loading