FrankChen021 commented on code in PR #19014:
URL: https://github.com/apache/druid/pull/19014#discussion_r4173099597
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/DefaultKubernetesTaskRunnerDynamicConfig.java:
##########
@@ -35,14 +35,17 @@ public class DefaultKubernetesTaskRunnerDynamicConfig
implements KubernetesTaskR
@JsonCreator
public DefaultKubernetesTaskRunnerDynamicConfig(
+ @Nullable
@JsonProperty("podTemplateSelectStrategy") PodTemplateSelectStrategy
podTemplateSelectStrategy,
+ @Nullable
@JsonProperty("capacity") Integer capacity
)
{
this.podTemplateSelectStrategy = podTemplateSelectStrategy;
this.capacity = capacity;
}
+ @Nullable
Review Comment:
[P1] Remove duplicate @Nullable annotation
**Finding:** The merge result still places two javax.annotation.Nullable
annotations on each getter: one before @Override and another after
@JsonProperty. Nullable is not repeatable, so javac rejects this class with a
duplicate-annotation error and the Kubernetes extension cannot compile.
**Suggestion:** Keep a single @Nullable annotation on each getter and remove
the duplicate from the merged declaration.
##########
extensions-core/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/KubernetesTaskExecutionConfigResource.java:
##########
@@ -57,16 +57,18 @@ public class KubernetesTaskExecutionConfigResource
private static final Logger log = new
Logger(KubernetesTaskExecutionConfigResource.class);
private final JacksonConfigManager configManager;
private final AuditManager auditManager;
- private AtomicReference<KubernetesTaskRunnerDynamicConfig> dynamicConfigRef
= null;
+ private final KubernetesTaskRunnerEffectiveConfig effectiveConfig;
@Inject
public KubernetesTaskExecutionConfigResource(
final JacksonConfigManager configManager,
- final AuditManager auditManager
+ final AuditManager auditManager,
+ final KubernetesTaskRunnerEffectiveConfig effectiveConfig
Review Comment:
[P2] Do not require single-cluster config for multik8s
**Finding:** Adding KubernetesTaskRunnerEffectiveConfig as a resource
dependency forces Guice to instantiate and validate
KubernetesTaskRunnerStaticConfig, whose namespace is @NotNull. A supported
multik8s deployment can omit druid.indexer.runner.namespace and provide only
clusters[*].taskNamespace; with this dependency, that configuration fails
validation while provisioning the resource, before the execution-config
endpoints can be used.
**Suggestion:** Make the resource depend only on configuration valid for
both runner types, or provide the active multik8s configuration when resolving
effective values.
--
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]
