Dennis-Mircea commented on code in PR #1253:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1253#discussion_r4512587665
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/bluegreen/BlueGreenUtils.java:
##########
@@ -379,9 +381,21 @@ public static FlinkDeployment prepareFlinkDeployment(
// Deployment metadata
ObjectMeta flinkDeploymentMeta =
getDependentObjectMeta(context.getBgDeployment());
flinkDeploymentMeta.setName(childDeploymentName);
- flinkDeploymentMeta.setLabels(
- Map.of(BlueGreenDeploymentType.LABEL_KEY,
blueGreenDeploymentType.toString()));
+ flinkDeploymentMeta.setLabels(childLabels(context,
blueGreenDeploymentType));
flinkDeployment.setMetadata(flinkDeploymentMeta);
return flinkDeployment;
}
+
+ /** Builds the child labels: the parent's labels plus the Blue/Green type
marker. */
+ private static Map<String, String> childLabels(
+ BlueGreenContext context, BlueGreenDeploymentType
blueGreenDeploymentType) {
+ Map<String, String> labels = new HashMap<>();
+ // Inherit the parent's labels so children match the operator's label
selector
Review Comment:
This inline comment is too verbose. There is already the method Javadoc that
mentions this.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/bluegreen/BlueGreenUtils.java:
##########
@@ -379,9 +381,21 @@ public static FlinkDeployment prepareFlinkDeployment(
// Deployment metadata
ObjectMeta flinkDeploymentMeta =
getDependentObjectMeta(context.getBgDeployment());
flinkDeploymentMeta.setName(childDeploymentName);
- flinkDeploymentMeta.setLabels(
- Map.of(BlueGreenDeploymentType.LABEL_KEY,
blueGreenDeploymentType.toString()));
+ flinkDeploymentMeta.setLabels(childLabels(context,
blueGreenDeploymentType));
flinkDeployment.setMetadata(flinkDeploymentMeta);
return flinkDeployment;
}
+
+ /** Builds the child labels: the parent's labels plus the Blue/Green type
marker. */
+ private static Map<String, String> childLabels(
+ BlueGreenContext context, BlueGreenDeploymentType
blueGreenDeploymentType) {
+ Map<String, String> labels = new HashMap<>();
+ // Inherit the parent's labels so children match the operator's label
selector
+
Optional.ofNullable(context.getBgDeployment().getMetadata().getLabels())
+ .ifPresent(labels::putAll);
+ // The canary marker would make the FlinkDeployment controller skip
the child
+ labels.remove(CanaryResourceManager.CANARY_LABEL);
Review Comment:
I'm not so sure about this addition.. A FlinkDeployment marked as canary has
no spec, and its only purpose is for the operator health flow. Promoting a
canary to a normal resource is not something that we want to do. The best thing
will be to reject upfront a Blue/Green deployment to wrap canaries.
As part of #1234 PR I'm proposing an improvement to restrict the canaries to
FlinkDeployment resources only. Still, this doesn't cover the cases where there
is a FlinkDeployment wrapped within a FlinkBlueGreenDeployment. So, in my
opinion, this should be tackled as a separate PR, a follow-up to #1234, where
we exclude this explicitly.
--
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]
