Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DFA Get Stats can return multiple responses if more than one error #60900

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

davidkyle
Copy link
Member

If the search for get stats for multiple jobs fail the listener is called for each failure. This change waits for all responses then returns the first error if there was one.

This solves half the problem reported in #60876 which also serves as test coverage for the change as it is very difficult to test directly

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@droberts195 droberts195 added v7.9.1 and removed v7.9.0 labels Aug 10, 2020
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@dimitris-athanasiou
Copy link
Contributor

We follow the same pattern in TransportGetJobsStatsAction.gatherStatsForClosedJobs too. Might be worth fixing the same issue there.

@davidkyle
Copy link
Member Author

We follow the same pattern in TransportGetJobsStatsAction.gatherStatsForClosedJobs

😱 how did we miss that

@davidkyle
Copy link
Member Author

@elasticmachine update branch

@davidkyle
Copy link
Member Author

run elasticsearch-ci/packaging-sample-windows

@Mpdreamz
Copy link
Member

Mpdreamz commented Sep 2, 2020

@davidkyle was this backported to 7.9? I only see a commit linked against 7.x

@davidkyle
Copy link
Member Author

was this backported to 7.9?

Erm no and I've missed 7.9.1 FF now.
This isn't a critical bug it can bypass 7.9 and go directly to 7.10 I've updated the labels accordingly.

Thanks for the ping 🙏

lcawl added a commit to lcawl/elasticsearch that referenced this pull request Sep 3, 2020
lcawl added a commit that referenced this pull request Sep 3, 2020
lcawl added a commit that referenced this pull request Sep 3, 2020
@droberts195
Copy link
Contributor

This isn't a critical bug it can bypass 7.9 and go directly to 7.10 I've updated the labels accordingly.

As a result of not backporting this to 7.9 we still have test failures in 7.9, for example https://gradle-enterprise.elastic.co/s/5rmxilu75h5ik

So I think either this fix should be backported to 7.9 regardless of whether it will get released or else that test should be muted in 7.9.

@davidkyle
Copy link
Member Author

Those recent failures are caused by a leaking test and should have been fixed by #61180 but that change was also not backported. I've raised #62054 to backport to 7.9.

The failure fixed in this PR is very rare, I don't think the backport is necessary at this stage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants