Remove test usages of getDefaultBackingIndexName in CCR tests
#127693
Conversation
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
| protected static boolean indexExists(String index) throws IOException { | ||
| Response response = adminClient().performRequest(new Request("HEAD", "/" + index)); | ||
| return RestStatus.OK.getStatus() == response.getStatusLine().getStatusCode(); | ||
| } |
There was a problem hiding this comment.
This method was already defined (with a better implementation) in ESRestTestCase.
| /** | ||
| * 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); |
There was a problem hiding this comment.
The idea here is nice, but it doesn't completely avoid any issues, as the cluster(s) don't use this time supplier.
| 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 was a problem hiding this comment.
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.
nielsbauman
added
>test
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
I added the |
There was a problem hiding this comment.
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());
We replace usages of time sensitive
DataStream#getDefaultBackingIndexNamewith 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