Copilot commented on code in PR #6820:
URL: https://github.com/apache/hive/pull/6820#discussion_r4099685449
##########
packaging/src/kubernetes/config/rbac/cluster-role.yaml:
##########
@@ -41,7 +41,7 @@ rules:
- apiGroups: [""]
resources: ["events"]
verbs: ["create", "patch"]
- # Pods: read-only for readiness checking
+ # Pods: read for readiness checking + delete for bulk update rollout
- apiGroups: [""]
resources: ["pods"]
- verbs: ["get", "list", "watch"]
+ verbs: ["get", "list", "watch", "delete"]
Review Comment:
The raw-manifest role still omits `patch`, unlike the Helm role.
`HiveClusterAutoscaler.updateDeploymentPodDeletionCost` calls
`pods(...).edit(...)` during autoscaling, so raw-manifest installations will
receive Forbidden errors and lose deletion-cost-based scale-down behavior;
include `patch` here as well.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/model/spec/LlapSpec.java:
##########
@@ -129,7 +137,8 @@ public record LlapTezAmSpec(
envVars = envVars != null ? envVars : List.of();
autoscaling = autoscaling != null ? autoscaling : new AutoscalingSpec(
false, 0, 1, 20, 60, 900, 600, 10, 0, 0, null);
- tezAm = tezAm != null ? tezAm : new LlapTezAmSpec(null, null, null, null);
+ tezAm = tezAm != null ? tezAm : new LlapTezAmSpec(null, null, null, null,
null);
+ updateStrategy = updateStrategy != null ? updateStrategy : "RollingUpdate";
Review Comment:
`LlapSpec` creates a non-null `LlapTezAmSpec` here, and that nested
constructor defaults its `updateStrategy` to `RollingUpdate`. Therefore the
fallback in `HiveDependentResource.tezAmUpdateStrategy()` is never reached when
the per-LLAP override is omitted, so a top-level `spec.tezAm.updateStrategy:
Recreate` is silently ignored for every per-LLAP TezAM Deployment. Preserve the
distinction between an omitted nested override and an explicit value when
resolving this precedence.
##########
packaging/src/kubernetes/src/java/org/apache/hive/kubernetes/operator/dependent/HiveDependentResource.java:
##########
@@ -976,4 +978,27 @@ private static String buildJmxExporterConfig(String
component) {
return sb.toString();
}
+ protected static DeploymentStrategy buildDeploymentUpdateStrategy(String
updateStrategy) {
+ String strategy = updateStrategy != null ? updateStrategy :
"RollingUpdate";
+ if ("Recreate".equalsIgnoreCase(strategy)) {
+ return new DeploymentStrategyBuilder().withType("Recreate").build();
+ }
+ return new DeploymentStrategyBuilder()
+ .withType("RollingUpdate")
Review Comment:
Every value other than `Recreate` is silently converted to `RollingUpdate`,
even though the new API documents only those two values. A typo or unsupported
strategy therefore changes rollout semantics without an error. Validate the
value (preferably with an enum in the CRD and model validation) and reject
unknown values consistently for Deployments and StatefulSets.
--
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]
