mateczagany commented on code in PR #791:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/791#discussion_r1517507683
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/AbstractFlinkService.java:
##########
@@ -1134,4 +1141,28 @@ private Configuration
getOperatorRestConfig(Configuration origConfig) throws IOE
});
return conf;
}
+
+ protected void scaleJmToZero(
+ EditReplacePatchable<Deployment> jmDeployment, String namespace,
String clusterId) {
+ LOG.info("Scaling down JM deployment to 0 before deletion");
Review Comment:
Since we also have a possible timeout here, maybe it's worth adding the
duration of the timeout in the log
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/service/StandaloneFlinkServiceTest.java:
##########
@@ -90,14 +90,12 @@ public void testDeleteClusterDeployment() throws Exception {
assertEquals(2, deployments.size());
- var requestsBeforeDelete = mockServer.getRequestCount();
flinkStandaloneService.deleteClusterDeployment(
flinkDeployment.getMetadata(),
flinkDeployment.getStatus(),
new Configuration(),
false);
- assertEquals(2, mockServer.getRequestCount() - requestsBeforeDelete);
Review Comment:
For the JM deletion, instead of 1 DELETE request, this will cost 3 requests:
- Patch resource
- `waitUntilCondition` gets resource, if the condition is not fulfilled,
starts websocket (so this might be 2 requests)
- Delete resource
And we still have the TM deletion request below the new logic, so this will
be 4 requests total instead of 2.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/AbstractFlinkService.java:
##########
@@ -1134,4 +1141,28 @@ private Configuration
getOperatorRestConfig(Configuration origConfig) throws IOE
});
return conf;
}
+
+ protected void scaleJmToZero(
+ EditReplacePatchable<Deployment> jmDeployment, String namespace,
String clusterId) {
+ LOG.info("Scaling down JM deployment to 0 before deletion");
+ try {
+
+ // We use patching instead of scaling to avoid the need for new
permissions
+ var patch = new
DeploymentBuilder().editOrNewSpec().withReplicas(0).endSpec().build();
+ jmDeployment.patch(PatchContext.of(PatchType.JSON_MERGE), patch);
+ kubernetesClient
+ .pods()
+ .inNamespace(namespace)
+
.withLabels(KubernetesUtils.getJobManagerSelectors(clusterId))
Review Comment:
`getJobManagerSelectors()` will not work for standalone clusters, as the
`type` label will be different. This should be
`StandaloneKubernetesUtils#getJobManagerSelectors` in that case.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
