Skip to content

Conversation

@pxsalehi
Copy link
Member

@pxsalehi pxsalehi commented Mar 3, 2023

We have some YAML tests that would require at least one
replica (search shard) to run with Stateless and since they
wait for green, they explicitly set replicas to 0 (see e.g.
realtime_refresh). Using 2 nodes
by default makes sure we could run those tests w/o any
changes. IMO, they are pretty important/essential tests.

Relates #94303

@pxsalehi pxsalehi marked this pull request as ready for review March 6, 2023 08:31
@pxsalehi pxsalehi added >test Issues or PRs that are addressing/adding tests :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Mar 6, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Mar 6, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@pxsalehi pxsalehi requested review from mark-vieira and tlrx March 6, 2023 08:38
Copy link
Contributor

@idegtiarenko idegtiarenko left a comment

Choose a reason for hiding this comment

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

👍

(Not required now, just an idea to discuss) Is it also possible to override this in any possible way?
I think it would be great to have something (a system property?) to configure minimum/default amount of nodes for the build if not configured explicitly. This way stateless build would be ready for any possible new modules and statefull will keep using less nodes.

@pxsalehi
Copy link
Member Author

pxsalehi commented Mar 6, 2023

(Not required now, just an idea to discuss) Is it also possible to override this in any possible way? I think it would be great to have something (a system property?) to configure minimum/default amount of nodes for the build if not configured explicitly. This way stateless build would be ready for any possible new modules and statefull will keep using less nodes.

I think it is possible but the main issue is that the YAML tests are not that flexible to assert expectations or setup the test conditionally (e.g. based on whether the target cluster is stateless or stateful).

@mark-vieira
Copy link
Contributor

mark-vieira commented Mar 6, 2023

It's not clear to me whether this is only required for stateless or for core elasticsearch as well. If the former, then we can simply configure the cluster when testing stateless to have multiple nodes. There's no reason to add additional overhead to testing core elasticsearch if it's not strictly required.

@pxsalehi
Copy link
Member Author

pxsalehi commented Mar 6, 2023

It's not clear to me whether this is only required for stateless or for core elasticsearch as well. If the former, then we can simply configure the cluster when testing stateless to have multiple nodes. There's no reason to add additional overhead to testing core elasticsearch if it's not strictly required.

@mark-vieira I understand. The problem is that some of the YAML tests (which are shared between the two) have a wait_for_status: green. This wouldn't work on stateful if I change replica to 1. It wouldn't work on stateless if I have replica set to 0 (no search shards). Maybe those wait_for_status: greens are unnecessary. I could try to remove that part, but that might make the test flaky!

@mark-vieira
Copy link
Contributor

@mark-vieira I understand. The problem is that some of the YAML tests (which are shared between the two) have a wait_for_status: green. This wouldn't work on stateful if I change replica to 1. It wouldn't work on stateless if I have replica set to 0 (no search shards). Maybe those wait_for_status: greens are unnecessary. I could try to remove that part, but that might make the test flaky!

Right, I don't see how changing the way the cluster is formed in stateful is relevant. If we need multiple nodes when running these shared tests in stateless, then we'll update how the cluster is configured there.

I think the bit that is getting missed here is that the YAML is shared between the two, but the code you're updating in this PR is not.

@pxsalehi
Copy link
Member Author

pxsalehi commented Mar 6, 2023

As discussed w/ @mark-vieira on another channel, the change is required ALSO on stateful since the tests that are update in this PR would fail otherwise.

@pxsalehi pxsalehi merged commit def4426 into elastic:main Mar 6, 2023
pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Mar 8, 2023
elasticsearchmachine pushed a commit that referenced this pull request Mar 8, 2023
)

I have reviewed the tests that motivated the use of a two node cluster
for the YAML tests. It seems there is no reason anymore to use
`wait_for_status: green` and `number_of_replicas: 0` since the [default
values](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-index_.html#index-wait-for-active-shards)
make sure the write will succeed. There are many newer YAML tests where
an index creation is followed by an index operation, w/o waiting for
green. With this PR, I'm reverting the changes in
#94304, and instead modify
the tests.
pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants