jtuglu-netflix commented on code in PR #18082:
URL: https://github.com/apache/druid/pull/18082#discussion_r2141830484
##########
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:
Hmm, I actually think it's the opposite: if we have a strict identity
requirement for a property (like the `id` in the spec, or `supervisorId` in the
task spec) that's being used for identifying things (i.e which supervisor we're
talking about), we shouldn't leave it up to the implementing classes to decide
what's a sensible default (we know what the only backwards-compatible default
is – `dataSource`).
In the case of the `*Supervisor`, I'm fine with leaving the custom ID (it
currently works and means less change area) since it isn't involved in any real
identification. We still have access to the real ID via `spec.getId()`. I think
the only change would be to use
```java
StringUtils.format("KafkaSupervisor-%s", spec.getId())
```
instead of:
```java
StringUtils.format("KafkaSupervisor-%s",
spec.getDataSchema().getDataSource())
```
--
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]
