Maybe like this? #1
Conversation
| s -> Optional.ofNullable(s).map(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER::parse).map(Instant::from), | ||
| validator, | ||
| properties | ||
| ); |
There was a problem hiding this comment.
I modified this to make it more clear that this was optional rather than trying to use a sentinel value. If would have just returned null but settings don't like to return null.
There was a problem hiding this comment.
I find it's rare used like Setting<Optional<...>>
There was a problem hiding this comment.
Yeah. I'm not super happy with that. Java's so used to returning null for optional stuff. But the Settings infra doesn't like it. I'm not sure what's right here, but I worry that any sentinel value is going to cause trouble.
There was a problem hiding this comment.
I'm not super attached to Optional here - I think it's uncommon. And you can see I'm just turning it into null. I guess another option is to use some kind of min and max instant value instead as unset.
| * For the most part this class concentrates on validating settings and | ||
| * mappings. Most different behavior is controlled by forcing settings | ||
| * to be set or not set and by enabling extra fields in the mapping. | ||
| */ |
There was a problem hiding this comment.
I've trying to keep this class mostly about validating settings and making mappers. That way the settings and the mappers go and do the work.
| IndexMetadata.INDEX_ROUTING_PATH, | ||
| IndexSettings.TIME_SERIES_START_TIME, | ||
| IndexSettings.TIME_SERIES_END_TIME | ||
| ), |
There was a problem hiding this comment.
I already was validating the mode=time_series requires routing_path and non-timeseries doesn't support them. So I figured it was cleaner to put time_series_start_time and end_time next to that validation.
| public Iterator<Setting<?>> settings() { | ||
| return IndexMode.VALIDATE_WITH_SETTINGS.iterator(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Because I moved the validation around MODE has to come after the START_TIME and END_TIME or else it can't validate against them. You get a sneaky NullPointerException on init which was fun to debug.
| * End time of the time_series index. | ||
| */ | ||
| private volatile long timeSeriesEndTime; | ||
| private volatile Instant timeSeriesEndTime; |
There was a problem hiding this comment.
I went with Instant here so that we can use it's nullability to turn the behavior on and off. It feels a little sneaky and probably worth a comment, but fairly ok.
| if (value >= endTime) { | ||
| throw new IllegalArgumentException("time series index @timestamp value [" + value + "] must be smaller than " + endTime); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method makes it fairly simple to handle millis and nanos using the "native" resolution.
| } | ||
| } | ||
| } | ||
| validate.validate(timestamp, context); |
There was a problem hiding this comment.
I put the validation here rather than in the DataStreamTimestampFieldMapper which I think is, well, probably wrong. At least, i think wrong for us. But it felt ok at the time. Honestly, now that i think about it, it's probably better to just call timestampFieldType.resolution().validateTimestamp(fields[0].value) off in DataSreamTimestampFieldMapper.
But, either way, I'm hoping to keep the validation linked to just the settings. And to have the validation "just happen" in the mappers. This way IndexMode isn't really involved directly. When you read this code you don't have to know that IndexMode exists, just that sometimes we want to validate the timestamp field.
There was a problem hiding this comment.
And, now that I think about it, adding the validateTimestamp method to resolution probably isn't worth it - the DataStreamTimestampFieldMapper can call convert no problem.
| } | ||
| settingRequiresTimeSeries(settings, IndexMetadata.INDEX_ROUTING_PATH); | ||
| settingRequiresTimeSeries(settings, IndexSettings.TIME_SERIES_START_TIME); | ||
| settingRequiresTimeSeries(settings, IndexSettings.TIME_SERIES_END_TIME); |
…uginFuncTest builds distribution from branches via archives extractedAssemble [bwcDistVersion: 8.3.0, bwcProject: staged, expectedAssembleTaskName: extractedAssemble, #1] elastic#119870
2 participants
I was thinking something like this. It's not finished at all, but I was thinking it'd be more inspiration.