Skip to content

Fix ReactiveStorageIT#testScaleDuringSplitOrClone#88607

Merged
fcofdez merged 5 commits intoelastic:mainfrom
fcofdez:fix-autoscaling-split-test
Aug 9, 2022
Merged

Fix ReactiveStorageIT#testScaleDuringSplitOrClone#88607
fcofdez merged 5 commits intoelastic:mainfrom
fcofdez:fix-autoscaling-split-test

Conversation

@fcofdez
Copy link
Copy Markdown
Contributor

@fcofdez fcofdez commented Jul 19, 2022

Sometimes the autoscaling decider returns an empty response when
the service does not have enough information to provide an autoscaling
decision, i.e. when a new node joins it tries to fetch the new node
memory info and this might take a while. This commits adds a busy
assertion to ensure that a valid autoscaling capacity is provided
eventually.

Closes #88478

Sometimes the autoscaling decider returns an empty response when
the service does not have enough information to provide an autoscaling
decision, i.e. when a new node joins it tries to fetch the new node
memory info and this might take a while. This commits adds a busy
assertion to ensure that a valid autoscaling capacity is provided
eventually.

Closes elastic#88478
@fcofdez fcofdez added >test Issues or PRs that are addressing/adding tests :Distributed/Autoscaling Automatically adding or removing nodes in a cluster Team:Distributed Meta label for distributed team. v8.4.0 labels Jul 19, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

// provides a complete autoscaling capacity response
assertBusy(() -> {
// validate initial state looks good
GetAutoscalingCapacityAction.Response response = capacity();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the NPE is just because there is no result in the response, why not only assertBusy getting the capacity() call, and leave the rest out of it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More of a question/nit! :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense, it just felt like the entire assertion block belonged together, but it would make the test slower if there's some other failure. I'll amend it 👍

@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:04
@fcofdez fcofdez requested a review from henningandersen July 25, 2022 09:00
@fcofdez
Copy link
Copy Markdown
Contributor Author

fcofdez commented Jul 26, 2022

@elasticmachine update branch

@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
Copy link
Copy Markdown
Member

@pxsalehi pxsalehi left a comment

Choose a reason for hiding this comment

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

LGTM!

@fcofdez
Copy link
Copy Markdown
Contributor Author

fcofdez commented Aug 9, 2022

@elasticmachine update branch

@fcofdez fcofdez merged commit 7b615ac into elastic:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Autoscaling Automatically adding or removing nodes in a cluster Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] ReactiveStorageIT testScaleDuringSplitOrClone failing

4 participants