kfaraz commented on code in PR #18082:
URL: https://github.com/apache/druid/pull/18082#discussion_r2140708529
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java:
##########
@@ -93,6 +96,7 @@ public SeekableStreamSupervisorSpec(
)
{
this.ingestionSchema = checkIngestionSchema(ingestionSchema);
+ this.id = Preconditions.checkNotNull(Configs.valueOrDefault(id,
ingestionSchema.getDataSchema().getDataSource()), "spec id cannot be null!");
Review Comment:
Yes, it does ensure uniform behaviour across sub-classes in terms of default
`id`.
Had this been a field other than `id`, the current approach would have
definitely been preferable.
But IMO, `id` is a first class citizen which captures the identity of the
object in question.
There are certain places where we are currently forced to allow null due to
backward compat.
Barring those places, we should make the `id` non-null everywhere else.
Keeping it non-null forces callers to declare upfront the identity of the
object they are creating,
which makes for readable code. Otherwise, we need to go all the way to the
super class constructor
to figure out how nulls would be handled.
(Compare this to `AbstractTask` which accepts a non-null `id` too.)
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
