-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Fix throttled reindex_from_remote #23953
Conversation
reindex_from_remote was using `TimeValue#toString` to generate the scroll timeout which is bad because that generates fractional time values that are useful for people but bad for Elasticsearch which doesn't like to parse them. This switches it to using `TimeValue#getStringRep` which spits out whole time values. Closes to elastic#23945 Makes elastic#23828 even more desirable
It seems strange that this is tested with a REST test, is there any way we could extract the scroll calculation into a (maybe static) method where it could be tested independently of the full reindex pipeline? (It'd also be good to keep the REST test too of course) |
I updated a unit test. I can change it if you like. I admit it is pretty
perfunctory.
…On Thu, Apr 6, 2017, 6:05 PM Lee Hinman ***@***.***> wrote:
It seems strange that this is tested with a REST test, is there any way we
could extract the scroll calculation into a (maybe static) method where it
could be tested independently of the full reindex pipeline? (It'd also be
good to keep the REST test too of course)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23953 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANLolGqfgU-qInsIgky6WO0FJDlu6qIks5rtWGggaJpZM4M2Ggo>
.
|
Yeah, it'd be nice if there was something like public void testCalculationThingys() throws Exception {
assertThat(ReindexCalculationThing.calculateTime(90, TimeUnit.SECONDS), equalTo("2m")); // instead of "1.5m"
} So this was a little more well encapsulated |
That should make it more clear that the parsing is what is what the other side is going to do so it is what has to work.
@dakrone I pushed some more changes - 2 unit test tweaks and another place where I was calling toString. I can push a concrete (rather than randomized) example too if you'd like. Right now the scroll tests are still embedded with the rest of the request building tests. I think splitting them out would make the whole thing more brittle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for changing the tests
Thanks for reviewing @dakrone! |
* master: Discovery EC2: Remove region setting (elastic#23991) AWS Plugins: Remove signer type setting (elastic#23984) Settings: Disallow secure setting to exist in normal settings (elastic#23976) Add registration of new discovery settings Settings: Migrate ec2 discovery sensitive settings to elasticsearch keystore (elastic#23961) Fix throttled reindex_from_remote (elastic#23953) Add comment why we check for null fetch results during merge
reindex_from_remote was using `TimeValue#toString` to generate the scroll timeout which is bad because that generates fractional time values that are useful for people but bad for Elasticsearch which doesn't like to parse them. This switches it to using `TimeValue#getStringRep` which spits out whole time values. Closes to #23945 Makes #23828 even more desirable
I think this has caused our remote reindexing to fail.
Indicating that our 1.4.5 server is not too happy about the "nanos". Is there a workaround that can I can deploy until it's fixed? |
@silashansen That's a duplicate of #24520. |
reindex_from_remote was using
TimeValue#toString
to generate thescroll timeout which is bad because that generates fractional
time values that are useful for people but bad for Elasticsearch
which doesn't like to parse them. This switches it to using
TimeValue#getStringRep
which spits out whole time values.Closes to #23945
Makes #23828 even more desirable.