Fix bug in faster interval rounding#56433
Merged
nik9000 merged 4 commits intoelastic:masterfrom May 8, 2020
Merged
Conversation
This fixes a bug in the interval rounding and two test bugs that showed up when I ran 1000s of iterations of the tests. The bug was that we could end up with duplicate transitions if we only needed the last transition from the list of fully defined transitions and the transitions overlapped with the rules. This happens. Its ok. We had defence against that if we needed more than one transition but forgot it in this case. Now we have it and assertions to make sure we catch any similar mistakes. One test bug had to do with the randomized test calling `round(round(min))` because sometimes the first `round` call will return a time less than min. Specifically if `min` is near daylight savings time. Anyway, this change fixes it by building an extra-wide prepared rounding just in case. The other test bug came up when adding a test for the real bug, namely that `assertRoundingAtOffset` made an assertion that wasn't true if the time to round ended up being in an overlap. This just drops that assertion. We have similar assertions in cases where we *know* what kind of offsets we're working with in other tests. Closes elastic#56400
Collaborator
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
nik9000
commented
May 8, 2020
| * Sometimes the rules duplicate the transitions. And | ||
| * duplicates confuse us. So we have to skip past them. | ||
| */ | ||
| minSecond = t.toEpochSecond() + 1; |
Member
Author
There was a problem hiding this comment.
This is the actual bug fix.
Member
Author
|
run elasticsearch-ci/docs |
not-napoleon
approved these changes
May 8, 2020
|
|
||
| public void testLastTransitionOverlapsRules() { | ||
| /* | ||
| * America/Tijuana's transitions overlap its rules and we have to make |
Member
There was a problem hiding this comment.
I don't understand the phrase "transitions overlap its rules"
Member
Author
|
run elasticsearch-ci/packaging-sample-unix-docker |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
May 8, 2020
This fixes a bug in the interval rounding and two test bugs that showed up when I ran 1000s of iterations of the tests. The bug was that we could end up with duplicate transitions if we only needed the last transition from the list of fully defined transitions and the transitions overlapped with the rules. This happens. Its ok. We had defence against that if we needed more than one transition but forgot it in this case. Now we have it and assertions to make sure we catch any similar mistakes. One test bug had to do with the randomized test calling `round(round(min))` because sometimes the first `round` call will return a time less than min. Specifically if `min` is near daylight savings time. Anyway, this change fixes it by building an extra-wide prepared rounding just in case. The other test bug came up when adding a test for the real bug, namely that `assertRoundingAtOffset` made an assertion that wasn't true if the time to round ended up being in an overlap. This just drops that assertion. We have similar assertions in cases where we *know* what kind of offsets we're working with in other tests. Closes elastic#56400
Member
Author
|
I'm backporting this in #56396. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a bug in the interval rounding and two test bugs that showed
up when I ran 1000s of iterations of the tests. The bug was that we
could end up with duplicate transitions if we only needed the last
transition from the list of fully defined transitions and the
transitions overlapped with the rules. This happens. Its ok. We had
defence against that if we needed more than one transition but forgot it
in this case. Now we have it and assertions to make sure we catch any
similar mistakes.
One test bug had to do with the randomized test calling
round(round(min))because sometimes the firstroundcall will returna time less than min. Specifically if
minis near daylight savingstime. Anyway, this change fixes it by building an extra-wide prepared
rounding just in case.
The other test bug came up when adding a test for the real bug, namely
that
assertRoundingAtOffsetmade an assertion that wasn't true if thetime to round ended up being in an overlap. This just drops that
assertion. We have similar assertions in cases where we know what kind
of offsets we're working with in other tests.
Closes #56400