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

Types removal - forward-compatible code for bulk API change coming in 7.0 #37105

Closed
wants to merge 1 commit into from

Conversation

markharwood
Copy link
Contributor

This rolling upgrade test now tolerates (but does not require) type removal warnings.
Mainly adding here to 6.x to make the master builds for my bulk types removal PR (36549) pass.

…ulk API.

This rolling upgrade test now tolerates (but does not require) type removal warnings.
Mainly adding here to 6.x to make the master builds for my bulk types removal PR (36549) pass.
@markharwood markharwood added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v6.7.0 labels Jan 3, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@jtibshirani
Copy link
Contributor

@markharwood do you have the reproduction line for the failing test motivating this change? I would like to dig into it a bit to make sure I understand. In the meantime I'll start to review #36549 so as not to hold you up.

@markharwood
Copy link
Contributor Author

markharwood commented Jan 3, 2019

I think the repro line was this one:

./gradlew :x-pack:qa:rolling-upgrade:with-system-key:v6.7.0#oneThirdUpgradedTestRunner -Dtests.seed=7EE10D0C67FF5836 -Dtests.class=org.elasticsearch.upgrades.IndexingIT -Dtests.method="testIndexing" -Dtests.security.manager=true -Dtests.locale=ar-BH -Dtests.timezone=US/Pacific-New -Dcompiler.java=11 -Druntime.java=8

Sadly it never reproduced on my macbook - it always failed with a different "timed out waiting for a connection" type error so I could only get it to fail in CI which made chasing it more tedious

@jtibshirani
Copy link
Contributor

Thanks for the line, unfortunately I also wasn't able to reproduce the issue. After taking a look, I'm still having trouble understanding how the 6.x version of this test could be affecting the results of the test on master, since I think only the master test should be running (even if some of the nodes in the test are on 6.x). I wonder if it's worth catching up with the core infra team to see if they can shed light on the failures you've been seeing?

@markharwood
Copy link
Contributor Author

markharwood commented Jan 4, 2019

@nik9000 Can you shed any light on the above?
I know you were involved with IndexingIT and deprecation warnings checks so can you explain why a PR in master would fail in the way described? The master server issues type removal warnings and the and the test client expects them but we see the RestClient barfing because of unexpected warnings - see here

My only thought was the test is running a 6.x client (which currently has no warnings expectations) against the 7.0 server.

@jtibshirani
Copy link
Contributor

jtibshirani commented Jan 4, 2019

@markharwood looking over the linked build failures, could the issue be that we need to add expectWarnings to the x-pack version of IndexingIT as well? Here's the bulk call, which I didn't see modified yet in your PR: https://github.com/elastic/elasticsearch/blob/master/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java#L138

@nik9000
Copy link
Member

nik9000 commented Jan 4, 2019

I'm still having trouble understanding how the 6.x version of this test could be affecting the results of the test on master, since I think only the master test should be running (even if some of the nodes in the test are on 6.x)

Unless something changed in the past few weeks, I think this statement is accurate. Our tests "look back" on previous versions.

@markharwood markharwood closed this Jan 7, 2019
@markharwood
Copy link
Contributor Author

Closed as redundant - underlying issue was identified in #37105 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants