gyfora commented on code in PR #481:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/481#discussion_r1048373787
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractFlinkResourceReconciler.java:
##########
@@ -316,6 +319,38 @@ private boolean checkNewSpecAlreadyDeployed(CR resource,
Configuration deployCon
return false;
}
+ /**
+ * Scale the cluster whenever there is a scaling change, based on the task
manager replica
+ * update or the parallelism in case of scheduler mode.
+ *
+ * @param cr Resource being reconciled.
+ * @param ctx Reconciliation context.
+ * @param observeConfig Observe configuration.
+ * @param deployConfig Configuration to be deployed.
+ * @param diffType Spec change type.
+ * @throws Exception
Review Comment:
missing `@return` javadoc
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/StandaloneFlinkService.java:
##########
@@ -171,7 +171,11 @@ protected void deleteClusterInternal(ObjectMeta meta,
boolean deleteHaConfigmaps
@Override
public boolean scale(ObjectMeta meta, JobSpec jobSpec, Configuration conf)
{
- if (conf.get(JobManagerOptions.SCHEDULER_MODE) == null) {
+ if (conf.get(JobManagerOptions.SCHEDULER_MODE) == null
+ &&
conf.get(StandaloneKubernetesConfigOptionsInternal.CLUSTER_MODE)
+ .equals(
+
StandaloneKubernetesConfigOptionsInternal.ClusterMode
+ .APPLICATION)) {
Review Comment:
Instead of checking the ClusterMode, it's simpler to check `jobSpec !=
null`, and also please explicitly check the `SCHEDULER_MODE != REACTIVE`
instead of just checking null
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/service/StandaloneFlinkServiceTest.java:
##########
@@ -184,6 +184,125 @@ public void testReactiveScale() throws Exception {
buildConfig(flinkDeployment, configuration)));
}
+ @Test
+ public void testTMReplicaScaleApplication() throws Exception {
Review Comment:
I think the new tests are good enough and you can simply remove the old unit
test. That is covered in the one you added
--
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]
