Refactor DataStreamLifecycle creation code
#138780
Conversation
nielsbauman
added
>refactoring
:Data Management/Data streams
elasticsearchmachine
added
v9.3.0
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
| return DataStreamLifecycle.createDataLifecycleTemplate( | ||
| frequently(), | ||
| randomResettable(ESTestCase::randomTimeValue), | ||
| downsampling, | ||
| randomResettable(() -> randomSamplingMethod(downsampling.get())) | ||
| ); | ||
| return DataStreamLifecycle.dataLifecycleBuilder() | ||
| .enabled(frequently()) | ||
| .dataRetention(randomResettable(ESTestCase::randomTimeValue)) | ||
| .downsamplingRounds(downsampling) | ||
| .downsamplingMethod(randomResettable(() -> randomSamplingMethod(downsampling.get()))) | ||
| .buildTemplate(); |
There was a problem hiding this comment.
There are one or two similar usages where we do actually lose a bit of value by using a builder. In these cases, it would actually be nice to get a compilation error if you add a new field but forget to add it to these methods that generate random instances. That way, you maintain full coverage by having to specify a random value for the new field here as well.
However, because DataStreamLifecycle.LifecycleType is package private, we can't currently use the constructor of DataStreamLifecycle. The only options I could think of are using a builder (like I currently do in this PR) or make the LifecycleType enum public. Thoughts are welcome :)
| enum LifecycleType implements Writeable { | ||
| DATA("data", (byte) 0), | ||
| FAILURES("failures", (byte) 1); | ||
|
|
||
| private final String label; | ||
| private final byte id; | ||
| private static final Map<Byte, LifecycleType> REGISTRY = Arrays.stream(LifecycleType.values()) | ||
| .collect(Collectors.toMap(l -> l.id, Function.identity())); | ||
|
|
||
| LifecycleType(String label, byte id) { | ||
| this.label = label; | ||
| this.id = id; | ||
| } | ||
| DATA, | ||
| FAILURES; | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.write(id); | ||
| out.writeEnum(this); | ||
| } | ||
|
|
||
| public static LifecycleType read(StreamInput in) throws IOException { | ||
| return REGISTRY.get(in.readByte()); | ||
| return in.readEnum(LifecycleType.class); |
There was a problem hiding this comment.
These changes are technically a bit out-of-scope for this PR, but I figured it's fine to include them here. There is no need to have this custom serialization; we have built-in code for that.
| ); | ||
| DataStreamFailureStore.Template result = DataStreamFailureStore.builder(template).composeTemplate(template).buildTemplate(); | ||
| assertThat(result, equalTo(normalise(template))); | ||
| assertThat(result, equalTo(template)); |
There was a problem hiding this comment.
By changing the builder fields to ResettableValue, we don't need to normalise these templates anymore.
| assertLifecycleResolution(service, project, List.of(ct30d, ctNullRetention), null, DataStreamLifecycle.Template.DATA_DEFAULT); | ||
| assertLifecycleResolution( | ||
| service, | ||
| project, | ||
| List.of(ct30d, ctNullRetention), | ||
| null, | ||
| DataStreamLifecycle.dataLifecycleBuilder().dataRetention(ResettableValue.reset()).buildTemplate() | ||
| ); |
There was a problem hiding this comment.
The assertions in this test were previously technically incorrect. With my change to ResettableValue in the builder, we can now properly assess these values. Previously, we weren't able to distinguish between ResettableValue.reset() and ResettableValue.undefined() because of how the template composition uses the builders (which wasn't necessarily a problem, as that doesn't matter for the final value, which is null in both cases), but now the end result does represent the correct intention of the value.
2 participants
We currently have several methods for creating
DataStreamLifecycleand correspondingDataStreamLifecycle.Templateobjects. These methods need to specify all fields of those objects, which results in a lot of changed lines when adding a new field.To avoid those extra changed lines, we convert most usages to using builders, which don't require passing
nullvalues to have default values for fields.