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

[ML] Add basic BWC tests for data frame analytics #47212

Conversation

dimitris-athanasiou
Copy link
Contributor

Adds basic BWC tests for data frame analytics jobs.

@dimitris-athanasiou dimitris-athanasiou added >test Issues or PRs that are addressing/adding tests :ml Machine learning v8.0.0 v7.5.0 labels Sep 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

One question though: Which fields of the "get" and "get stats" response should we verify?
Do you think it is worth adding more fields to increase coverage?

@dimitris-athanasiou
Copy link
Contributor Author

@przemekwitek Sure, why not? I'll add some more match clauses.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Do we want to test starting/stopping the analytics as well? I suppose it would start/stop very quickly, so validating state afterwards might not work, but we should at least verify that neither API throws an error.

@@ -0,0 +1,37 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

I think we should test putting analytics on the mixed cluster as well.

@dimitris-athanasiou
Copy link
Contributor Author

@benwtrent I thought about testing start/stop. We can't know how far the job went to decide whether we can restart it without conditionals, so we can only do so in one of the cluster states (old, mixed or upgraded). Thus, I don't see much value doing so. I have talked to QA asking them to add a test scenario where they start a longer running job and then perform a rolling upgrade to cover this.

@droberts195
Copy link
Contributor

We can't know how far the job went to decide whether we can restart it without conditionals, so we can only do so in one of the cluster states (old, mixed or upgraded)

I agree it's pointless to test start/stop in the old cluster as that's what the integration tests of that branch will be testing. But since we can test start/stop in one of the states I think mixed would be worthwhile, as that's unlikely to get tested in other places. It would be useful to find out if something we've changed prevents old analytics jobs from starting up in the mixed version cluster. That could be an early warning of something we've overlooked with the BWC. (And then I agree that since the analytics job has done something in the mixed cluster it cannot be started again in the fully upgraded one.)

I have talked to QA asking them to add a test scenario where they start a longer running job and then perform a rolling upgrade to cover this.

Yes, I think this is best to leave this more complex scenario to the QA tests. There will be fewer spurious failures caused by things like #36816 (comment).

@dimitris-athanasiou dimitris-athanasiou force-pushed the add-basic-bwc-tests-df-analytics branch from a201123 to a96c279 Compare October 4, 2019 11:07
@dimitris-athanasiou
Copy link
Contributor Author

I have updated this PR to include tests to put a job in the mixed cluster as well as start/stop jobs during that state.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

:shipit:

@dimitris-athanasiou dimitris-athanasiou force-pushed the add-basic-bwc-tests-df-analytics branch from a96c279 to 0982506 Compare October 8, 2019 14:27
@dimitris-athanasiou dimitris-athanasiou merged commit 2246767 into elastic:master Oct 8, 2019
@dimitris-athanasiou dimitris-athanasiou deleted the add-basic-bwc-tests-df-analytics branch October 8, 2019 16:33
benwtrent pushed a commit to benwtrent/elasticsearch that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >test Issues or PRs that are addressing/adding tests v7.5.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants