Skip to content

Conversation

@bgeng777
Copy link
Contributor

@bgeng777 bgeng777 commented Feb 21, 2022

  1. Refactor FlinkUtils#getEffectiveConfig into smaller pieces.
  2. Refactor some codes in TestUtils and add unit test for FlinkUtils#getEffectiveConfig.

@bgeng777
Copy link
Contributor Author

cc @wangyang0918 @gyfora @tweise

@wangyang0918
Copy link
Contributor

  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();

@bgeng777
Copy link
Contributor Author

bgeng777 commented Feb 21, 2022

Thanks a lot for the detailed review!

  1. Sure. It seems that the fix of JM replicas is just a piece of codes so I did it here. I will move it out and leave the work to the dedicated jira for better tracking.
  2. My bad. Check the KUBERNETES_POD_TEMPLATE field again and I understood it wrongly before.
  3. Yes, I also think about using Builder pattern when refactoring. Considering not to change the code style too much, I did not use it initially. But I believe it could make codes more neat and flexible. Updated.

Copy link
Contributor

@gyfora gyfora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor cosmetic comments, otherwise looks good :)

Copy link
Contributor

@tweise tweise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great to get some test coverage for this area.

@tweise
Copy link
Contributor

tweise commented Feb 21, 2022

Also see #10 for related field changes

Copy link
Contributor

@gyfora gyfora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you :)

Will merge this after breakfast

@asfgit asfgit closed this in 29ef2f5 Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants