Skip to content

Conversation

@nielsbauman
Copy link
Contributor

These tests do not seem valuable anymore, now that multi-project has been merged some time ago. Additionally, creating objects for a cluster state this way is tricky, as those objects might get new features/fields, which would break serialization.

Closes #130872

These tests do not seem valuable anymore, now that multi-project has
been merged some time ago. Additionally, creating objects for a cluster
state this way is tricky, as those objects might get new
features/fields, which would break serialization.

Closes elastic#130872
@nielsbauman nielsbauman requested a review from ywangd July 8, 2025 23:16
@nielsbauman nielsbauman added >test Issues or PRs that are addressing/adding tests :Data Management/Indices APIs APIs to create and manage indices and templates labels Jul 8, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Jul 8, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@ywangd
Copy link
Member

ywangd commented Jul 11, 2025

now that multi-project has been merged some time ago

I don't think this is a valid reason? In stateful, 9.0 and 8.19 are before multi-project and we must ensure 9.1 and future 9.x versions communicate with them correctly. So it seems to me that the test still has value till version 10.

The test failure is because cluster state with a non-default project cannot be serialized to a version that is before multi-project. This is in fact the correct behaviour. So I think we should fix it by catching and asserting the expected exceptions when the test randomization falls into this case.

@nielsbauman
Copy link
Contributor Author

In stateful, 9.0 and 8.19 are before multi-project and we must ensure 9.1 and future 9.x versions communicate with them correctly. So it seems to me that the test still has value till version 10.

As I understood it, these tests were mostly created because we didn't have any BwC tests running between main and PRs (I believe nowadays we do have those), and we wanted to ensure we didn't break serialization between main and the multi-project branch. What added value do you see in having these tests compared to the BwC integration tests we run against multiple versions?

Also, these tests only cover a small part of the cluster state serialization; they only create a data stream with some indices and some nodes. The BwC integration tests have far more elaborate coverage.

Finally, I tried to fix the test by using a different helper method to create the data stream, but that used features that were added recently, and thus breaks serialization altogether (because it's trying to serialize features that were introduced after MP was merged). I guess keeping the test setup unchanged and simply catching the exception for non-default projects is a simple fix, but as I said before, I'm not convinced yet on the value of these tests.

@ywangd
Copy link
Member

ywangd commented Jul 14, 2025

It's a more targeted version for testing BWC serializaiton. Yeah it should also be mostly covered by other more general BWC tests, except for maybe throwing UnsupportedOperationException for "There is 1 project, but it has id ...", i.e. the test failure. I think we could use a test for this scenario if we don't have it already. It does not have to be a full blown serialization test and something like this should fine. So maybe we could add such assertions along with removing this test class?

@nielsbauman
Copy link
Contributor Author

Thanks a lot for your input, @ywangd! I just added a new test in 136360e, but I'm not 100% if that matches what you had in mind. Let me know :)

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

@nielsbauman nielsbauman merged commit 004af14 into elastic:main Jul 15, 2025
32 of 33 checks passed
@nielsbauman nielsbauman deleted the fix-130872 branch July 15, 2025 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Indices APIs APIs to create and manage indices and templates Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] ClusterStateSerializationTests testSerializationPreMultiProject failing

3 participants