kfaraz commented on PR #18082:
URL: https://github.com/apache/druid/pull/18082#issuecomment-2972258973
Thanks for the update, @jtuglu-netflix !
1. Approach 1 regarding DB seems reasonable to me.
We should add a short comment in `SQLMetadataConnector` to this effect.
2. I see your point regarding defaulting to datasource name in the
`SeekableStreamSupervisor` class due to
the possible nullability of ingestion schema. We can proceed with the
approach you have.
3. We should continue to prefix the thread names with the supervisor type,
same as before.
Otherwise it is very difficult to figure out what a thread represents only
from the datasource name or supervisor ID.
4. There are some unaddressed comments from the previous review. Please
address those.
5. Please follow standard Druid log interpolation where applicable:
`Supervisor[%s]` (no space) is preferred over `Supervisor [%s]`.
6. Fix up formatting glitches, if any.
7. __Please avoid force pushing commits__ as it makes reviews difficult
#### Default value of `supervisorId`
Regarding the default `supervisorId` logic, as you had pointed out, there
are currently too many places that are trying to determine the default
supervisor id.
There are two things we could do to bring the logic in one place:
1. __Add a utility method__ like
`IdUtils.getStreamSupervisorId(supervisorId, dataSource)`.
- Use `Configs.valueOrDefault()` in the impl but at least all of the
defaulting logic will go though one place.
- Javadoc and null checks in just one place.
- It will be easy to track all the places that use this defaulting logic.
2. __Add a new `SupervisorId` class__ (perhaps better alternative)
- Same benefits as static utility method but better forces authors to do
the right thing.
- Plus classes like `IndexerMetadataStorageCoordinator` will not need to
do any additional validation
- Serde into a string or a full JSON object, whatever makes most sense.
- It will put to rest all the discussion on where we should compute the
default value (except now it will be where we should construct `SupervisorId`
object 😛 , but for that we can proceed with what you have right now)
Please let me know what you think. Will do a complete review after the above
are resolved.
--
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]
