wangyang0918 commented on pull request #9:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/9#issuecomment-1046798237


   > 1. Refactor `FlinkUtils#getEffectiveConfig` into smaller pieces.
   > 2. Fix the problem that the field of `replicas` in `JobManagerSpec` will 
not take effect.
   > 3. Fix the problem that the basic `podTemplate` in `FlinkDeploymentSpec` 
will not take effect if JM or TM spec does not specify `podTemplate` on their 
own.
   > 4. Refactor some codes in `TestUtils` and add unit test for 
`FlinkUtils#getEffectiveConfig`.
   
   * I am suggesting to factor out No.2 into a separate ticket.
   * Why we have the No.3 issue? IIUC, currently the pod template should work 
for both JobManager and TaskManager.
   * For No.1, what do you think about introducing a `FlinkConfigBuilder` just 
like following. It will make things easier to test.
   ```
   /** Build effective flink config from {@link FlinkDeployment} */
   public class FlinkConfigBuilder {
       private final FlinkDeployment deploy;
       private final Configuration effectiveConfig;
   
       public FlinkConfigBuilder(FlinkDeployment deploy) {
           this.deploy = deploy;
           this.effectiveConfig = FlinkUtils.loadConfiguration();
       }
   
       public FlinkConfigBuilder applyImageToConfig() {
           effectiveConfig.set(KubernetesConfigOptions.CONTAINER_IMAGE, 
deploy.getSpec().getImage());
           return this;
       }
   
       // Apply other fields to the config
       ... ...
   
       public Configuration build() {
           // Set some basic configuration
           final String namespace = deploy.getMetadata().getNamespace();
           final String clusterId = deploy.getMetadata().getName();
           effectiveConfig.setString(KubernetesConfigOptions.NAMESPACE, 
namespace);
           effectiveConfig.setString(KubernetesConfigOptions.CLUSTER_ID, 
clusterId);
           return effectiveConfig;
       }
   }
   ```
   
   ```
   final Configuration effectiveConfig = new 
FlinkConfigBuilder(flinkApp).applyImageToConfig().apply<...>.build();
   ```


-- 
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]


Reply via email to