Fix data stream retrieval in DataStreamLifecycleServiceIT
#125195
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -854,6 +854,32 @@ private static Settings.Builder getExcludeSettings(int num, Settings.Builder bui | |
| return builder; | ||
| } | ||
|
|
||
| /** | ||
| * Waits for the specified data stream to have the expected number of backing indices. | ||
| */ | ||
| public static List<String> waitForDataStreamBackingIndices(String dataStreamName, int expectedSize) { | ||
| return waitForDataStreamIndices(dataStreamName, expectedSize, false); | ||
| } | ||
|
|
||
| /** | ||
| * Waits for the specified data stream to have the expected number of backing or failure indices. | ||
| */ | ||
| public static List<String> waitForDataStreamIndices(String dataStreamName, int expectedSize, boolean failureStore) { | ||
|
There was a problem hiding this comment. Food for thought, what if instead of There was a problem hiding this comment. Yeah, I thought about that too. I decided to not do that (yet) as I didn't see any use cases where that would be relevant. On second thought, I think sections like these could make use of that: However, in that case, it doesn't really make sense to retrieve the backing indices. Then, having a different dedicated method just for data stream predicates feels a little over-specific, so a generic cluster-state predicate one is probably more sensible. However, then we're getting pretty close to the addTemporaryStateListener already (and the awaitClusterState). I would still see value in adding a wrapper in ESIntegTestCase that fetches the ClusterService instance from the master node, to avoid having to do that in every test. What do you think?
There was a problem hiding this comment. Hm, I would say it depends on how much is this going to be used, for example:
If you agree with the above, where do you think this should reside? There was a problem hiding this comment. I'll respond to those questions later, but I'm not sure how they're related to this discussion? I was referring to the implementation of the additional helper method. There was a problem hiding this comment. I'm going to merge this for now. There are other similar test suites that need to be updated. When I get to those, I'll automatically see what potential other use cases there are and we can adjust the helper method accordingly. There was a problem hiding this comment. ah, I misunderstood. What I had in mind was that we can share the method that you said would be rather specific:
Let me try to understand better what you meant with the following:
You mean to avoid doing this: Not sure, if this is worth it honestly, considering it's only one line of code. Do you think this is too much of the mental load for a developer? There was a problem hiding this comment. Note that we already have Not that I'm against a utility for this somewhere in the test framework (suggest There was a problem hiding this comment. Thanks, David! My only reason for
was based on my assumption that There was a problem hiding this comment. @gmarouli, FTR, I agree that the mental load is minimal. I have a personal vendetta with boilerplate/duplicate code. In general, I'm also a fan of reducing test method sizes, as I feel a lot of our tests are hard to read, so any line reduction is a win in my eyes. |
||
| final var clusterService = internalCluster().getCurrentMasterNodeInstance(ClusterService.class); | ||
| final var listener = ClusterServiceUtils.addTemporaryStateListener(clusterService, clusterState -> { | ||
|
There was a problem hiding this comment. David Turner suggested using a temporary cluster state listener instead of an There was a problem hiding this comment. I would rather not add the override that returns the cluster state like this. Instead, just await for the cluster to reach a suitable state and then use the regular APIs and transport actions to query what you need. That way you get the little bit of extra coverage from calling the transport action, and you also get some confidence that the state you were waiting for doesn't immediately vanish in the next update since the transport action may see a future state. There was a problem hiding this comment. Hm, that reduces the performance benefit because we'll have to do another request, but only by a little bit I think. I agree that it does bring back some extra coverage. I'll revert the changes to There was a problem hiding this comment. The performance benefits are all about avoiding the |
||
| final var dataStream = clusterState.metadata().getProject().dataStreams().get(dataStreamName); | ||
| if (dataStream == null) { | ||
| return false; | ||
| } | ||
| return dataStream.getDataStreamIndices(failureStore).getIndices().size() == expectedSize; | ||
| }); | ||
| final var state = safeAwait(listener); | ||
| // We will only reach the return statement when the data stream exists (and has the expected number of indices), | ||
| // so we can safely retrieve the data stream without worrying about NPEs. | ||
| final var indices = state.metadata().getProject().dataStreams().get(dataStreamName).getDataStreamIndices(failureStore).getIndices(); | ||
| return indices.stream().map(Index::getName).toList(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a list of the data stream's backing index names. | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit on the fence about backporting these changes or not. These fixes are only necessary on
v9.1.0because the GET data streams API only runs on the local node starting with that version, but I'm thinking about backporting test fixes in the future to versions beforev9.1.0. I think I'm leaning towards not backporting, but other thoughts are welcome.