kfaraz commented on code in PR #16613:
URL: https://github.com/apache/druid/pull/16613#discussion_r1641256091
##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/SegmentMetadataEvent.java:
##########
@@ -85,7 +86,7 @@ public SegmentMetadataEvent(
)
{
this.dataSource = dataSource;
- this.createdTime = createdTime;
+ this.createdTime = createdTime != null ? createdTime : DateTimes.nowUtc();
Review Comment:
I agree, it makes sense to ensure that the `timestamp` is always non-null.
But if it is important for the field to be non-null, then we should be more
explicit about the value we use for it. We should probably avoid silently
converting a null to "now" (though there are other places in the Druid code
that do it 😅 ).
So I would advise to do the following:
- Retain the public constructor but do a validation of this field thus
forcing callers to pass a non-null value.
- Remove arg `eventTime` from method `.create()` and always use
`DateTime.nowUtc()`.
- Add a javadoc to both the constructor and `create()` clarifying this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
