uds5501 commented on code in PR #18341:
URL: https://github.com/apache/druid/pull/18341#discussion_r2244931558
##########
indexing-service/src/main/java/org/apache/druid/guice/IndexingServiceTaskLogsModule.java:
##########
@@ -39,13 +42,22 @@ public class IndexingServiceTaskLogsModule implements Module
public void configure(Binder binder)
{
PolyBind.createChoice(binder, "druid.indexer.logs.type",
Key.get(TaskLogs.class), Key.get(FileTaskLogs.class));
+ PolyBind.createChoice(binder, "druid.indexer.logs.delegate.type",
Key.get(TaskLogs.class, Names.named("delegate")), Key.get(FileTaskLogs.class));
Review Comment:
This sounds good, in my head I am looking to provide the flexibility just
for log types for now. The `delegated` / `reportType` could take care of other
task responsibilities. So finally, imo it should look like one of these.
### Option 1 [asking for explicit types for each responsibility]
```
druid.indexer.logs.type=switching
druid.indexer.logs.switching.reportsType=file
druid.indexer.logs.switching.payloadManagerType=file
druid.indexer.logs.switching.logsHandlerType=switching
druid.indexer.logs.switching.logHandler.logsPushType=noop
druid.indexer.logs.switching.logHandler.logsStreamType=file
```
translating to
```java
public class SwitchingTaskLogs implements TaskLogs
{
private final TaskLogs reportDelegate;
private final TaskLogs payloadDelegate;
private final TaskLogs logDelegate;
@Inject
public SwitchingTaskLogs(
@Named("report") TaskLogs reportDelegate,
@Named("payload") TaskLogs payloadDelegate,
@Named("log") TaskLogs logDelegate
)
{
this.reportDelegate = reportDelegate;
this.payloadDelegate = payloadDelegate;
this.logDelegate = logDelegate;
}
}
public class TaskLogManager implements TaskLogs // this / some flavour of
switching task log will be injected as switching log delegate to handle the
task related responsibilities with an appropriate factory placed in front.
{
private final TaskLogStreamer streamer;
private final TaskLogPusher pusher;
@Inject
public TaskLogManager(
@Named("taskLogPusher") TaskLogs pusher,
@Named("taskLogStreamer") TaskLogs streamer,
)
{
this.pusher = pusher;
this.streamer = streamer;
}
}
```
or we could create one like
### Option 2 [just keeping log handler separate while keeping the rest of
the responsibilities with the delegate class]
```
druid.indexer.logs.type=switching
druid.indexer.logs.switching.delegate=file
druid.indexer.logs.switching.logsHandlerType=switching
druid.indexer.logs.switching.logHandler.logsPushType=noop
druid.indexer.logs.switching.logHandler.logsStreamType=file
```
```java
public class SwitchingTaskLogs implements TaskLogs
{
private final TaskLogs delegate;
private final TaskLogs switchingLogDelegate;
@Inject
public SwitchingTaskLogs(
@Named("default") TaskLogs defaultDelegate,
@Named("log") TaskLogs logDelegate,
)
{
this.defaultDelegate = defaultDelegate;
this.logDelegate = logDelegate;
}
}
// the TaskLogManager stays the same.
```
**I am personally leaning towards option 2 given the usecase in hand. what
do you folks think?**
--
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]
