YongGang commented on PR #16510:
URL: https://github.com/apache/druid/pull/16510#issuecomment-2153326191
> -1 on this change because I do not understand the use of the new
interfaces. It seems like what we want here is a config that is scoped to the
pod template adapter, but this PR introduces configs for general use across all
of the k8s extension. Because of this the names of the interfaces and their
uses are not clear to me eg. ExecutionConfig that returns an
ExecutionBehaviorStrategy that gets a "category" from the task. It seems like
all we need here is something like a `PodTemplateNamingStrategy` or a
`PodTemplateSelector`
>
> The other thing that feels clunky is the selector class. It should be an
interface so that it can be extended in the future. The current selector class
implementation does not provide good errors to users if they reference a field
that is not currently supported - like group id. It feels like the Selector
class is trying to implement a `Predicate<Task>`
>
> I'd recommend introducing an interface called `PodTemplateSelector` that
returns a PodTemplate given a Task object (similar to the BehaviorSelector
classes introduced in this patch). For the `Selectors` - I'd recommend renaming
them to `Matchers` that implement `Predicate<Task>`. We could then introduce
`and`, `not`, `or` matchers and matchers that match on dataSource, tags, any
context, task type, etc. The config would then look like
>
> ```
> "podTemplateSelectorStrategy" : {
> "type": matcherBased,
> "templateMatchers": [
> {
> "template": "template0",
> "matcher": {
> "type": or,
> "matchers": [
> {
> "type": "dataSource",
> "matchingNames": ["ds0"]
> },
> {
> "type": "context",
> "field": "myContextKey"
> "matchingNames": ["anyValue"]
> },
> ]
> }
> },
> {
> "template": "template1",
> "matcher": {
> "type": "taskType",
> "matchingTypes": ["index_kafka", "index_kinesis"]
> }
> }
> ]
> }
> ```
@suneet-s I have addressed most of the comments except for the `Matchers`
proposal. While it’s a good suggestion, I don’t believe it’s the right fit for
our scenario for a couple of reasons:
1. **Complexity and Applicability of Operators**: `Matchers` provide a
variety of operators such as `and`, `not`, and `or` for evaluation. Our current
design with sequence-based selectors is already complex and requires careful
attention to detail. Introducing additional operators would further complicate
the understanding of the dynamic config. Specifically, the `not` and `or`
operators might not be very useful for our focused criteria of template
selection. For example, the `not` operator could yield unexpected results,
especially as new Task types are continuously added, potentially covering
unintended scenarios without careful usage.
2. **Relevance of Typed Matchers**: While the typed `Matcher` approach is
excellent for extensibility and appears future-proof, it is more aligned with
scenarios like Druid’s Filter, where many implementations exist for type safety
and performance enhancements. In our case, however, the field comparisons are
predominantly string-based. Given that our main input in this K8s extension is
a **Task**, we do not anticipate needing significantly different criteria from
what is currently covered. Should future requirements drastically diverge,
adopting a new strategy would be more appropriate than extending the current
one.
Additionally, implementing `Matchers` would require introducing many small
Jackson classes, which could bloat the codebase. Our current implementation of
Selector is cleaner and more streamlined in comparison.
--
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]
