[Rollup] Remove builders from DateHistogramGroupConfig#32555
Merged
tlrx merged 4 commits intoelastic:masterfrom Aug 3, 2018
Merged
[Rollup] Remove builders from DateHistogramGroupConfig#32555tlrx merged 4 commits intoelastic:masterfrom
tlrx merged 4 commits intoelastic:masterfrom
Conversation
tlrx
added
review
v7.0.0
>refactoring
:StorageEngine/Rollup
Collaborator
|
Pinging @elastic/es-search-aggs |
jimczi
approved these changes
Aug 1, 2018
Contributor
There was a problem hiding this comment.
Can you add javadocs to this constructor and maybe add the fact that we use Joda to parse the timeZone. Also the timeZone is optional so we could maybe have a simple ctr for field and interval and one that have all the options ?
Contributor
There was a problem hiding this comment.
++
Also, could we annotate timeZone as @Nullable here too?
Member
Author
There was a problem hiding this comment.
I added some javadoc. Let me know if that's ok for you.
polyfractal
reviewed
Aug 1, 2018
Contributor
There was a problem hiding this comment.
Isn't this sort of test covered by AbstractSerializingTestCase via it's super-class AbstractWireTestCase? Or am I missing something that makes this different?
Member
Author
There was a problem hiding this comment.
It's a bit different: AbstractSerializingTestCase takes care of testing the XContent parsing logic and AbstractWireTestCase tests the java serialization logic. But this pull request changes the format of the timezone from DateTimeZone to String and AbstractWireTestCase will now execute the tests using the new type only.
This test checks that we can still read from / write to the old format that serializes/deserializes DateTimeZone objects.
Contributor
There was a problem hiding this comment.
Ooh, missed that little detail. Great thanks!
polyfractal
reviewed
Aug 1, 2018
Contributor
There was a problem hiding this comment.
Since timezone is now a string, we should probably check for Strings.isNullOrEmpty() instead of just null now.
(or null/empty check, I'll leave that up to personal preference :) )
polyfractal
approved these changes
Aug 1, 2018
Contributor
polyfractal
left a comment
polyfractal
left a comment
There was a problem hiding this comment.
Few minor comments/questions from me too but otherwise LGTM :)
jimczi
approved these changes
Aug 2, 2018
tlrx
force-pushed
the
remove-builders-from-rollup-fate-histo-group-config
branch
from
65d3ecb to
e75e3ec
Compare
tlrx
force-pushed
the
remove-builders-from-rollup-fate-histo-group-config
branch
from
e75e3ec to
ee78d03
Compare
Member
Author
|
Thanks @jimczi and @polyfractal |
tlrx
added a commit
that referenced
this pull request
Aug 3, 2018
Same motivation as #32507 but for the DateHistogramGroupConfig configuration object. This pull request also changes the format of the time zone from a Joda's DateTimeZone to a simple String. It should help to port the API to the high level rest client and allows clients to not be forced to use the Joda Time library. Serialization is impacted but does not need a backward compatibility layer as DateTimeZone are serialized as String anyway. XContent also expects a String for timezone, so I found it easier to move everything to String. Related to #29827
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Aug 6, 2018
…pe-detection-with-leading-whitespace * elastic/master: (34 commits) Cross-cluster search: preserve cluster alias in shard failures (elastic#32608) Handle AlreadyClosedException when bumping primary term [TEST] Allow to run in FIPS JVM (elastic#32607) [Test] Add ckb to the list of unsupported languages (elastic#32611) SCRIPTING: Move Aggregation Scripts to their own context (elastic#32068) Painless: Use LocalMethod Map For Lookup at Runtime (elastic#32599) [TEST] Enhance failure message when bulk updates have failures [ML] Add ML result classes to protocol library (elastic#32587) Suppress LicensingDocumentationIT.testPutLicense in release builds (elastic#32613) [Rollup] Update wire version check after backport Suppress Wildfly test in FIPS JVMs (elastic#32543) [Rollup] Improve ID scheme for rollup documents (elastic#32558) ingest: doc: move Dot Expander Processor doc to correct position (elastic#31743) [ML] Add some ML config classes to protocol library (elastic#32502) [TEST]Split transport verification mode none tests (elastic#32488) Core: Move helper date formatters over to java time (elastic#32504) [Rollup] Remove builders from DateHistogramGroupConfig (elastic#32555) [TEST} unmutes SearchAsyncActionTests and adds debugging info [ML] Add Detector config classes to protocol library (elastic#32495) [Rollup] Remove builders from MetricConfig (elastic#32536) ...
dnhatn
added a commit
that referenced
this pull request
Aug 6, 2018
* 6.x: [Kerberos] Use canonical host name (#32588) Cross-cluster search: preserve cluster alias in shard failures (#32608) [TEST] Allow to run in FIPS JVM (#32607) Handle AlreadyClosedException when bumping primary term [Test] Add ckb to the list of unsupported languages (#32611) SCRIPTING: Move Aggregation Scripts to their own context (#32068) (#32629) [TEST] Enhance failure message when bulk updates have failures [ML] Add ML result classes to protocol library (#32587) Suppress LicensingDocumentationIT.testPutLicense in release builds (#32613) [Rollup] Improve ID scheme for rollup documents (#32558) Mutes failing SQL string function tests due to #32589 Suppress Wildfly test in FIPS JVMs (#32543) Add cluster UUID to Cluster Stats API response (#32206) [ML] Add some ML config classes to protocol library (#32502) [TEST]Split transport verification mode none tests (#32488) [Rollup] Remove builders from DateHistogramGroupConfig (#32555) [ML] Add Detector config classes to protocol library (#32495) [Rollup] Remove builders from MetricConfig (#32536) Fix race between replica reset and primary promotion (#32442) HLRC: Move commercial clients from XPackClient (#32596) Security: move User to protocol project (#32367) Minor fix for javadoc (applicable for java 11). (#32573) Painless: Move Some Lookup Logic to PainlessLookup (#32565) Core: Minor size reduction for AbstractComponent (#32509) INGEST: Enable default pipelines (#32286) (#32591) TEST: Avoid merges in testSeqNoAndCheckpoints [Rollup] Remove builders from HistoGroupConfig (#32533) fixed elements in array of produced terms (#32519) Mutes ReindexFailureTests.searchFailure dues to #28053 Mutes LicensingDocumentationIT due to #32580 Remove the SATA controller from OpenSUSE box [ML] Rename JobProvider to JobResultsProvider (#32551)
dnhatn
added a commit
that referenced
this pull request
Aug 6, 2018
* master: Cross-cluster search: preserve cluster alias in shard failures (#32608) Handle AlreadyClosedException when bumping primary term [TEST] Allow to run in FIPS JVM (#32607) [Test] Add ckb to the list of unsupported languages (#32611) SCRIPTING: Move Aggregation Scripts to their own context (#32068) Painless: Use LocalMethod Map For Lookup at Runtime (#32599) [TEST] Enhance failure message when bulk updates have failures [ML] Add ML result classes to protocol library (#32587) Suppress LicensingDocumentationIT.testPutLicense in release builds (#32613) [Rollup] Update wire version check after backport Suppress Wildfly test in FIPS JVMs (#32543) [Rollup] Improve ID scheme for rollup documents (#32558) ingest: doc: move Dot Expander Processor doc to correct position (#31743) [ML] Add some ML config classes to protocol library (#32502) [TEST]Split transport verification mode none tests (#32488) Core: Move helper date formatters over to java time (#32504) [Rollup] Remove builders from DateHistogramGroupConfig (#32555) [TEST} unmutes SearchAsyncActionTests and adds debugging info [ML] Add Detector config classes to protocol library (#32495) [Rollup] Remove builders from MetricConfig (#32536) Tests: Add rolling upgrade tests for watcher (#32428) Fix race between replica reset and primary promotion (#32442)
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
5 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.
Same motivation as #32507 but for the
DateHistogramGroupConfigconfiguration object. This pull request also changes the format of the time zone from a Joda'sDateTimeZoneto a simpleString.It should help to port the API to the high level rest client and allows clients to not be forced to use the Joda Time library. Serialization is impacted but does not need a backward compatibility layer as DateTimeZone are serialized as String anyway. XContent also expects a String for timezone, so I found it easier to move everything to String but I'm happy to have any feedback on this change.
Related to #29827