Remove timeStampField parameter from DataStream constructors. #85309
Conversation
Timestamp field is always '@timestamp' and is already fixed to this field. Removing this parameter from DataStream's constructors makes this clearer and makes the code a bit cleaner too. This change doesn't break bwc in any way or change the xcontent/binary format. A next followup change may do this. Followup from elastic#85260
martijnvg
added
>non-issue
:Data Management/Data streams
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
| @@ -73,7 +74,8 @@ | |||
|
|
|||
| private final LongSupplier timeProvider; | |||
| private final String name; | |||
| private final TimestampField timeStampField; | |||
| // This was always fixed to @timestamp with the idea that one day this field could be configurable. This idea no longer exists. | |||
| private final TimestampField timeStampField = TIMESTAMP_FIELD; | |||
There was a problem hiding this comment.
Nitpick: If this is not going to be configurable, shouldn't we use the constant within the class and remove the field?
There was a problem hiding this comment.
Yes, that makes sense. The constant can be returned in the getter.
| @@ -589,6 +518,11 @@ public DataStream(StreamInput in) throws IOException { | |||
| ); | |||
| } | |||
|
|
|||
| static List<Index> readIndices(StreamInput in) throws IOException { | |||
| in.readString(); // timestamp field, which is always @timestamp | |||
There was a problem hiding this comment.
I was wondering why you chose to always read and ignore the timestamp field instead of using the version of ES to slowly remove the field from here.
There was a problem hiding this comment.
I didn't want to change serialization logic in this PR.
I will do this in a follow up pr as part of a larger change that also changes xcontent, but that change is breaking and I wanted to discuss this internally before going ahead with this.
martijnvg
added
the
auto-merge-without-approval
Timestamp field is always '@timestamp' and is already fixed to this field.
Removing this parameter from DataStream's constructors makes this clearer
and makes the code a bit cleaner too.
This change doesn't break bwc in any way or change the xcontent/binary format.
A next followup change may do this.
Followup from #85260