kfaraz commented on code in PR #18082:
URL: https://github.com/apache/druid/pull/18082#discussion_r2141862406
##########
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java:
##########
@@ -63,6 +63,7 @@ public RabbitStreamSupervisorSpec(
@JacksonInject SupervisorStateManagerConfig supervisorStateManagerConfig)
{
super(
+ id,
Review Comment:
I think we are going in circles with this 😛 😛 !
> Not saying this is a sensible default for all SupervisorSpec.
Agreed.
That's why the super constructor should indicate that it is desirable to
pass a non-null id.
Requiring a non-null value for `id` forces the future implementers of this
class to think
what the `id` should be rather than defaulting to some behaviour in the
super class that
is not readily apparent.
> hence why I'm insisting on a standard dataSource default in the absence of
this field being non-existent in the spec
It makes to have a default for back compat but this default behaviour should
not be a standard that applies
to future implementers too. It is just something we are doing for back
compat of existing specs.
And even for that, we shouldn't hide that logic in the super constructor
since this is an `id` field which
directly affects the end user. For any other non-identifying field, it would
have been fine.
e.g. for the `supervisorId` field in the `*Task` classes, it is probably
fine.
My biggest annoyance with passing a null is that two supervisors of
completely different types
but same datasource would end up having the same `id`.
Having the sub-class pass a non null `id` at least makes the sub class aware
of this fact.
All that said, in the end, we will still get the same result either way,
i.e. a non-null `id` for the spec 😛
It is just a matter of cleaner code which is easier to follow and maintain
in the long run,
even if it might come at the cost of a few duplicated lines.
If you feel really strongly about it, you may retain your logic in the super
`Spec` constructor.
- Please make sure to call out the default behaviour in the javadocs clearly.
- Make the logging key fix as suggested above since it is really confusing
right now.
--
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]
