FrankChen021 commented on code in PR #19014:
URL: https://github.com/apache/druid/pull/19014#discussion_r4165718234
##########
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 annotations
**Finding:** The merge result places two javax.annotation.Nullable
annotations on each getter: the newly added annotation before @Override and the
existing annotation after @JsonProperty. Nullable is not repeatable, so javac
rejects this class with a duplicate-annotation error and the Kubernetes
extension cannot compile.
**Suggestion:** Keep one @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 namespace for multik8s
**Finding:** Adding KubernetesTaskRunnerEffectiveConfig as a resource
dependency forces Guice to instantiate KubernetesTaskRunnerStaticConfig, whose
namespace is @NotNull. A supported multik8s deployment can omit
druid.indexer.runner.namespace and provide only clusters[*].taskNamespace; in
that configuration this resource dependency fails validation before the
execution-config endpoints can be used.
**Suggestion:** Make the resource depend only on configuration that is valid
for both runner types, or provide the active multik8s configuration when
resolving the 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]
