Fix AutoIntervalDateHistogram.testReduce random failures #32301
Conversation
|
Pinging @elastic/es-search-aggs |
| * The current implementation probably should not be invoked in a tight loop. | ||
| * @return Array of RoundingInfo | ||
| */ | ||
| RoundingInfo[] buildRoundings() { |
There was a problem hiding this comment.
Another thing we could do here (to avoid having to create an instance of the builder in the InternalAutoDateHistogramTests) is to make the signature here: static RoundingInfo[] buildRoundings(DateTimeZone) and then pass the time zone into it the method both in innerBuild() and in InternalAutoDateHistogramTests. We will also need to make createRounding(DateTimeUnit, DateTimeZone) static as well but thats fine I think.
| roundingInfos[5] = new RoundingInfo(Rounding.builder(DateTimeUnit.YEAR_OF_CENTURY).build(), 1, 10, 20, 50, 100); | ||
| AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); | ||
| // TODO[PCS]: timezone set automagically here? | ||
| roundingInfos = aggregationBuilder.buildRoundings(); |
There was a problem hiding this comment.
I'm actually not sure how the time zone would be picked up here which makes me think that its not being used currently and we are testing with only UTC at the moment so we probably need to fix that but we could do that in a follow up PR and just pass in UTC to create the roundings for this PR. I also think we need to create the roundings in createTestInstance() since because that method should be randomising the time zone the roundings may be different for each instance.
There was a problem hiding this comment.
Looking at InternalDateHistogramTeests we don't seem to randomise the time zone there either which is probably why it was missed here. Not randomising the time zone there is less of a problem because the time zone is not used directly in InternalDateHistogram
|
@colings86 I believe I've addressed your comments, this is ready for re-review. |
There was a problem hiding this comment.
@pcsanwald I left one last small comment but otherwise this LGTM once the build passes
| } | ||
|
|
||
| @Override | ||
| protected InternalAutoDateHistogram createTestInstance(String name, | ||
| List<PipelineAggregator> pipelineAggregators, | ||
| Map<String, Object> metaData, | ||
| InternalAggregations aggregations) { | ||
| AutoDateHistogramAggregationBuilder aggregationBuilder = new AutoDateHistogramAggregationBuilder("_name"); |
There was a problem hiding this comment.
I don't think we need to create a builder here anymore. These tests don't currently set the time zone in the builder anyway so aggregationBuilder.timeZone() will always be null which implicitly means the time zone will be treated as UTC. As mentioned before we should add a test that does set the time zone to test its treated correctly but that doesn't need to be part of this PR so I think we can remove this line and just pass in null below?
|
jenkins test this please |
|
@pcsanwald I set off another build as the failure seemed to be with building artifacts for the bwc version and seems to be a transient failure to me |
|
@elasticmachine test this please |
|
@elasticmachine test this please |
|
Just as FYI: We had a test failing on 6.x (https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+6.x+multijob-unix-compatibility/os=debian/1214/console) so I just wanted to ensure this gets backported to 6.x as well. |
|
@danielmitterdorfer I will backport today! had a few intermittent failures on this branch yesterday, hence the delay in merging. |
* master: Logging: Make node name consistent in logger (#31588) Mute SSLTrustRestrictionsTests on JDK 11 Increase max chunk size to 256Mb for repo-azure (#32101) Docs: Fix README upgrade mention (#32313) Changed ReindexRequest to use Writeable.Reader (#32401) Mute KerberosAuthenticationIT Fix AutoIntervalDateHistogram.testReduce random failures (#32301) fix no=>not typo (#32463) Mute QueryProfilerIT#testProfileMatchesRegular() HLRC: Add delete watch action (#32337) High-level client: fix clusterAlias parsing in SearchHit (#32465) Fix calculation of orientation of polygons (#27967) [Kerberos] Add missing javadocs (#32469) [Kerberos] Remove Kerberos bootstrap checks (#32451) Make get all app privs requires "*" permission (#32460) Switch security to new style Requests (#32290) Switch security spi example to new style Requests (#32341) Painless: Add PainlessConstructor (#32447) update rollover to leverage write-alias semantics (#32216) Update Fuzzy Query docs to clarify default behavior re max_expansions (#30819) INGEST: Clean up Java8 Stream Usage (#32059) Ensure KeyStoreWrapper decryption exceptions are handled (#32464)
* ccr: (24 commits) Remove _xpack from CCR APIs (elastic#32563) TEST: Avoid merges in testRecoveryWithOutOfOrderDelete Logging: Make node name consistent in logger (elastic#31588) Mute SSLTrustRestrictionsTests on JDK 11 Increase max chunk size to 256Mb for repo-azure (elastic#32101) Docs: Fix README upgrade mention (elastic#32313) Changed ReindexRequest to use Writeable.Reader (elastic#32401) Mute KerberosAuthenticationIT Fix AutoIntervalDateHistogram.testReduce random failures (elastic#32301) fix no=>not typo (elastic#32463) Mute QueryProfilerIT#testProfileMatchesRegular() HLRC: Add delete watch action (elastic#32337) High-level client: fix clusterAlias parsing in SearchHit (elastic#32465) Fix calculation of orientation of polygons (elastic#27967) [Kerberos] Add missing javadocs (elastic#32469) [Kerberos] Remove Kerberos bootstrap checks (elastic#32451) Make get all app privs requires "*" permission (elastic#32460) Switch security to new style Requests (elastic#32290) Switch security spi example to new style Requests (elastic#32341) Painless: Add PainlessConstructor (elastic#32447) ...
Closes #32215. Couple of bugs were uncovered during investigation:
RoundingsInfowere inconsistent between the test and implementation. To fix this, I refactored the tests to fetch roundings from the implementation.In generating the expected result, the test was not correctly respecting innerIntervals in rounding. Since re-using the logic from the
AutoDateHistogramAggregatoris a fairly large refactor, I've replicated some of the logic in thetestReducemethod.Can be reproduced using steps in #32215, I also tested a number of times with different seeds to convince myself I didn't break other things.