Aggregations: deprecate pre_zone and post_zone in date_histogram

[cbuescher]

The options for pre_zone and post_zone in date_histogram are going to be removed in 2.0 (see #9062) in favor of the already existing time_zone option. This commit deprecates those fields using ParseFields and adds deprecation notice to the migration guide.

[cbuescher]

@jpountz Some very small addendum to the clean up of the date_histogram. Will this be enough in terms of migration notice or do we need more?

[rjernst]

Either remove "the" before date_histogram, or add "aggregation" after.

[rjernst]

@cbuescher A couple minor grammar comments. Also, shouldn't the documentation for date_histogram note the deprecation of pre_zone and post_zone, not just the migration guide?

[cbuescher]

Thanks for the comments, changed the migration notes and added a deprecation note in the docs. However I'm not entirely sure how these kind of changes are normally handled in the docs. Since the options are still valid I thought it best to keep the old docs unchanged, so I only added a warning at the end.

[rjernst]

@cbuescher I think the deprecation warning should happen where the parameters are introduced. Unfortunately the docs there right now are very mixed together. Perhaps move the note to the beginning of the section on timezone, and slim the message down to something like "Only use time_zone, the other time zone related parameters are deprecated."

[jpountz]

+1 to @rjernst 's suggestion.

[cbuescher]

i just realized there is not much space in these deprecated[x.x.x, ...] sections, otherwise the layout seems to break (at least when I build the docs locally). Thats why I added two sections. Do you know of any better way to add a longer deprecation section to the docs to guide the user how to deal with the proposed changes?

[jpountz]

@cbuescher maybe you can use sections notifications as described here: https://github.com/elasticsearch/docs#section-notifications ?

[cbuescher]

Will do. Also I forgot to mark the pre/postZone methods in the Builder as deprecated, will push those changes also in a sec.

[cbuescher]

Tried using the section deprecation notes as suggested but this to me looked weired, just printed a warning below the section title. Without rewriting the documentation text this looks like the whole feature is deprecated. I split the notification into two now, added them above the relevant sections in the current text.
Also added java deprecations to the DateHistogramBuilder where relevant.

[colings86]

should we use ParseField.withAllDeprecated(String) here? That seems to be the way to specify that a field is completely deprecated. I think having the preferredName as "" might cause issues?

[cbuescher]

You mean like new ParseField("pre_zone").withAllDeprecated("") ? Since the argument of withAllDeprecated seems to replace the original names this would have to be empty. As long as having the deprecated name as preferedName still raises the desired exception, that would work, right?

[colings86]

I could be wrong but it looks like the argument to withAllDeprecated indicates the new setting which has replaced the deprecated options and is only used in the error message to point the user to the new option. so for pre_zone we would use new ParseField("pre_zone").withAllDeprecated("time_zone"). It might be worth checking with @s1monw that this is indeed how it should work

[cbuescher]

Yes, for that case makes sense. But for options like in the above case post_zone which is dropped entirely, wouldn't then be empty argument to withAllDeprecated() be the right thing? Since there is no other option replacing it.

[cbuescher]

Changed the usage of ParseField for the deprecated fields. "pre_offset" and "post_offset" now point to "offset", "pre_zone" and "post_zone" to "time_zone" as new field name when strict parsing is enforced with an exception message like Deprecated field [pre_zone] used, replaced by [time_zone]

For the "pre_zone_adjust_large_interval" I used empty name for the deprecation name, since this option is completely going to be removed. This leads to exception messages like Deprecated field [pre_zone_adjust_large_interval] used, replaced by []. I could also go along and change that message in ParseField if an option is deleted, that is allReplacedWith in ParseField is empty.

[jpountz]

LGTM. This is good enough for me.

[cbuescher]

@jpountz thanks, will push this then.

[cbuescher]

Pushed to 1.x branch with 4505aba.

[spalger]

@cbuescher would you mind taking a look at elastic/kibana#3566? We were previously using pre_zone_adjust_large_interval to get nicely rounded buckets at large intervals, but now our buckets are rounded to UTC standards rather than the local timezone. Not sure where the issue really is though :/