Skip to content

Commit c2d3510

Browse files
tlrxtlrx
committed
Apply feedback
1 parent 15e4bec commit c2d3510

File tree

7 files changed

+79
-37
lines changed

7 files changed

+79
-37
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfig.java‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,21 +75,41 @@ public class DateHistogramGroupConfig implements Writeable, ToXContentObject {
7575
private final DateHistogramInterval delay;
7676
private final String timeZone;
7777

78+
/**
79+
* Create a new {@link DateHistogramGroupConfig} using the given field and interval parameters.
80+
*/
81+
public DateHistogramGroupConfig(final String field, final DateHistogramInterval interval) {
82+
this(field, interval, null, null);
83+
}
84+
85+
/**
86+
* Create a new {@link DateHistogramGroupConfig} using the given configuration parameters.
87+
* <p>
88+
* The {@code field} and {@code interval} are required to compute the date histogram for the rolled up documents.
89+
* The {@code delay} is optional and can be set to {@code null}. It defines how long to wait before rolling up new documents.
90+
* The {@code timeZone} is optional and can be set to {@code null}. When configured, the time zone value is resolved using
91+
* ({@link DateTimeZone#forID(String)} and must match a time zone identifier provided by the Joda Time library.
92+
* </p>
93+
* @param field the name of the date field to use for the date histogram (required)
94+
* @param interval the interval to use for the date histogram (required)
95+
* @param delay the time delay (optional)
96+
* @param timeZone the id of time zone to use to calculate the date histogram (optional). When {@code null}, the UTC timezone is used.
97+
*/
7898
public DateHistogramGroupConfig(final String field,
7999
final DateHistogramInterval interval,
80100
final @Nullable DateHistogramInterval delay,
81-
final String timeZone) {
101+
final @Nullable String timeZone) {
82102
if (field == null || field.isEmpty()) {
83103
throw new IllegalArgumentException("Field must be a non-null, non-empty string");
84104
}
85105
if (interval == null) {
86-
throw new IllegalArgumentException("Interval must be a non-null, non-empty string");
106+
throw new IllegalArgumentException("Interval must be non-null");
87107
}
88108

89109
this.interval = interval;
90110
this.field = field;
91111
this.delay = delay;
92-
this.timeZone = timeZone != null ? timeZone : DEFAULT_TIMEZONE;
112+
this.timeZone = (timeZone != null && timeZone.isEmpty() == false) ? timeZone : DEFAULT_TIMEZONE;
93113

94114
// validate interval
95115
createRounding(this.interval.toString(), this.timeZone);

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,13 +125,18 @@ public void testValidateMatchingField() {
125125
assertThat(e.validationErrors().size(), equalTo(0));
126126
}
127127

128+
/**
129+
* Tests that a DateHistogramGroupConfig can be serialized/deserialized correctly after
130+
* the timezone was changed from DateTimeZone to String.
131+
*/
128132
public void testBwcSerialization() throws IOException {
129133
for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {
130134
final DateHistogramGroupConfig reference = ConfigTestHelpers.randomDateHistogramGroupConfig(random());
131135

132136
final BytesStreamOutput out = new BytesStreamOutput();
133137
reference.writeTo(out);
134138

139+
// previous way to deserialize a DateHistogramGroupConfig
135140
final StreamInput in = out.bytes().streamInput();
136141
DateHistogramInterval interval = new DateHistogramInterval(in);
137142
String field = in.readString();
@@ -147,6 +152,7 @@ public void testBwcSerialization() throws IOException {
147152
final DateHistogramInterval delay = randomBoolean() ? ConfigTestHelpers.randomInterval() : null;
148153
final DateTimeZone timezone = randomDateTimeZone();
149154

155+
// previous way to serialize a DateHistogramGroupConfig
150156
final BytesStreamOutput out = new BytesStreamOutput();
151157
interval.writeTo(out);
152158
out.writeString(field);

‎x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public class RollupJobIdentifierUtilTests extends ESTestCase {
3737
public void testOneMatch() {
3838
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
3939
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
40-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
40+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
4141
job.setGroupConfig(group.build());
4242
RollupJobCaps cap = new RollupJobCaps(job.build());
4343
Set<RollupJobCaps> caps = singletonSet(cap);
@@ -52,7 +52,7 @@ public void testOneMatch() {
5252
public void testBiggerButCompatibleInterval() {
5353
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
5454
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
55-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
55+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
5656
job.setGroupConfig(group.build());
5757
RollupJobCaps cap = new RollupJobCaps(job.build());
5858
Set<RollupJobCaps> caps = singletonSet(cap);
@@ -67,7 +67,7 @@ public void testBiggerButCompatibleInterval() {
6767
public void testIncompatibleInterval() {
6868
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
6969
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
70-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d"), null, null));
70+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d")));
7171
job.setGroupConfig(group.build());
7272
RollupJobCaps cap = new RollupJobCaps(job.build());
7373
Set<RollupJobCaps> caps = singletonSet(cap);
@@ -100,7 +100,7 @@ public void testBadTimeZone() {
100100
public void testMetricOnlyAgg() {
101101
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
102102
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
103-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
103+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
104104
job.setGroupConfig(group.build());
105105
job.setMetricsConfig(singletonList(new MetricConfig("bar", singletonList("max"))));
106106
RollupJobCaps cap = new RollupJobCaps(job.build());
@@ -115,7 +115,7 @@ public void testMetricOnlyAgg() {
115115
public void testOneOfTwoMatchingCaps() {
116116
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
117117
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
118-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
118+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
119119
job.setGroupConfig(group.build());
120120
RollupJobCaps cap = new RollupJobCaps(job.build());
121121
Set<RollupJobCaps> caps = singletonSet(cap);
@@ -132,7 +132,7 @@ public void testOneOfTwoMatchingCaps() {
132132
public void testTwoJobsSameRollupIndex() {
133133
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
134134
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
135-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
135+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
136136
group.setTerms(null);
137137
group.setHisto(null);
138138
job.setGroupConfig(group.build());
@@ -142,7 +142,7 @@ public void testTwoJobsSameRollupIndex() {
142142

143143
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2");
144144
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
145-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
145+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
146146
group2.setTerms(null);
147147
group2.setHisto(null);
148148
job2.setGroupConfig(group.build());
@@ -162,7 +162,7 @@ public void testTwoJobsSameRollupIndex() {
162162
public void testTwoJobsButBothPartialMatches() {
163163
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
164164
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
165-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
165+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
166166
job.setGroupConfig(group.build());
167167
job.setMetricsConfig(singletonList(new MetricConfig("bar", singletonList("max"))));
168168
RollupJobCaps cap = new RollupJobCaps(job.build());
@@ -171,7 +171,7 @@ public void testTwoJobsButBothPartialMatches() {
171171

172172
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2");
173173
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
174-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
174+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
175175
job2.setGroupConfig(group.build());
176176
job.setMetricsConfig(singletonList(new MetricConfig("bar", singletonList("min"))));
177177
RollupJobCaps cap2 = new RollupJobCaps(job2.build());
@@ -190,15 +190,15 @@ public void testTwoJobsButBothPartialMatches() {
190190
public void testComparableDifferentDateIntervals() {
191191
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
192192
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
193-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
193+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
194194
.setHisto(null)
195195
.setTerms(null);
196196
job.setGroupConfig(group.build());
197197
RollupJobCaps cap = new RollupJobCaps(job.build());
198198

199199
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2").setRollupIndex(job.getRollupIndex());
200200
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
201-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d"), null, null))
201+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d")))
202202
.setHisto(null)
203203
.setTerms(null);
204204
job2.setGroupConfig(group2.build());
@@ -219,15 +219,15 @@ public void testComparableDifferentDateIntervals() {
219219
public void testComparableDifferentDateIntervalsOnlyOneWorks() {
220220
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
221221
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
222-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
222+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
223223
.setHisto(null)
224224
.setTerms(null);
225225
job.setGroupConfig(group.build());
226226
RollupJobCaps cap = new RollupJobCaps(job.build());
227227

228228
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2").setRollupIndex(job.getRollupIndex());
229229
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
230-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d"), null, null))
230+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1d")))
231231
.setHisto(null)
232232
.setTerms(null);
233233
job2.setGroupConfig(group2.build());
@@ -248,15 +248,15 @@ public void testComparableDifferentDateIntervalsOnlyOneWorks() {
248248
public void testComparableNoHistoVsHisto() {
249249
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
250250
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
251-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
251+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
252252
.setHisto(null)
253253
.setTerms(null);
254254
job.setGroupConfig(group.build());
255255
RollupJobCaps cap = new RollupJobCaps(job.build());
256256

257257
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2").setRollupIndex(job.getRollupIndex());
258258
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
259-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
259+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
260260
.setHisto(new HistogramGroupConfig(100L, "bar"))
261261
.setTerms(null);
262262
job2.setGroupConfig(group2.build());
@@ -278,15 +278,15 @@ public void testComparableNoHistoVsHisto() {
278278
public void testComparableNoTermsVsTerms() {
279279
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
280280
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
281-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
281+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
282282
.setHisto(null)
283283
.setTerms(null);
284284
job.setGroupConfig(group.build());
285285
RollupJobCaps cap = new RollupJobCaps(job.build());
286286

287287
RollupJobConfig.Builder job2 = ConfigTestHelpers.getRollupJob("foo2").setRollupIndex(job.getRollupIndex());
288288
GroupConfig.Builder group2 = ConfigTestHelpers.getGroupConfig();
289-
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
289+
group2.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
290290
.setHisto(null)
291291
.setTerms(new TermsGroupConfig("bar"));
292292
job2.setGroupConfig(group2.build());

‎x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public void testBadQuery() {
120120
public void testRange() {
121121
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
122122
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
123-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
123+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
124124
job.setGroupConfig(group.build());
125125
RollupJobCaps cap = new RollupJobCaps(job.build());
126126
Set<RollupJobCaps> caps = new HashSet<>();
@@ -133,7 +133,7 @@ public void testRange() {
133133
public void testRangeNullTimeZone() {
134134
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
135135
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
136-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
136+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
137137
job.setGroupConfig(group.build());
138138
RollupJobCaps cap = new RollupJobCaps(job.build());
139139
Set<RollupJobCaps> caps = new HashSet<>();
@@ -146,7 +146,7 @@ public void testRangeNullTimeZone() {
146146
public void testRangeWrongTZ() {
147147
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
148148
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
149-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
149+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
150150
job.setGroupConfig(group.build());
151151
RollupJobCaps cap = new RollupJobCaps(job.build());
152152
Set<RollupJobCaps> caps = new HashSet<>();
@@ -190,7 +190,7 @@ public void testTermsQuery() {
190190
public void testCompounds() {
191191
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
192192
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
193-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
193+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
194194
job.setGroupConfig(group.build());
195195
RollupJobCaps cap = new RollupJobCaps(job.build());
196196
Set<RollupJobCaps> caps = new HashSet<>();
@@ -206,7 +206,7 @@ public void testCompounds() {
206206
public void testMatchAll() {
207207
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
208208
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
209-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
209+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
210210
job.setGroupConfig(group.build());
211211
RollupJobCaps cap = new RollupJobCaps(job.build());
212212
Set<RollupJobCaps> caps = new HashSet<>();
@@ -218,7 +218,7 @@ public void testMatchAll() {
218218
public void testAmbiguousResolution() {
219219
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
220220
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
221-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
221+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
222222
group.setTerms(new TermsGroupConfig("foo"));
223223
job.setGroupConfig(group.build());
224224
RollupJobCaps cap = new RollupJobCaps(job.build());
@@ -369,7 +369,7 @@ public void testLiveOnlyCreateMSearch() {
369369
public void testGood() {
370370
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
371371
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
372-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null));
372+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
373373
job.setGroupConfig(group.build());
374374
RollupJobCaps cap = new RollupJobCaps(job.build());
375375
Set<RollupJobCaps> caps = singletonSet(cap);
@@ -440,7 +440,7 @@ public void testGoodButNullQuery() {
440440
public void testTwoMatchingJobs() {
441441
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
442442
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
443-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
443+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
444444
.setHisto(null)
445445
.setTerms(null);
446446
job.setGroupConfig(group.build());
@@ -490,7 +490,7 @@ public void testTwoMatchingJobs() {
490490
public void testTwoMatchingJobsOneBetter() {
491491
RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo");
492492
GroupConfig.Builder group = ConfigTestHelpers.getGroupConfig();
493-
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, null))
493+
group.setDateHisto(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")))
494494
.setHisto(null)
495495
.setTerms(null);
496496
job.setGroupConfig(group.build());

0 commit comments

Comments
 (0)