zentol commented on a change in pull request #13190:
URL: https://github.com/apache/flink/pull/13190#discussion_r473220233
##########
File path:
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/cli/KubernetesSessionCli.java
##########
@@ -88,6 +88,7 @@ public Configuration getEffectiveConfiguration(String[] args)
throws CliArgsExce
private int run(String[] args) throws FlinkException, CliArgsException {
final Configuration configuration =
getEffectiveConfiguration(args);
+
KubernetesClusterClientFactory.ensureClusterIdIsSet(configuration);
Review comment:
The `(Kubernetes)ClusterDescriptor` provides no way of accessing the
cluster-id.
There is `ClusterClientFactory#getClusterId`, but this one only works the
way we'd like to after `ClusterClientFactory#createClusterDescriptor` was
called (since it modifies the passed configuration).
I thought about changing `ClusterClientFactory#getClusterId` to also set the
id if it is missing, to ensure that the behavior of the methods is identical
regardless of method call order, but I cannot assess what repercussions this
might have on users of the `ClusterClientFactory` interface.
Basically, I hate this behavior, and didn't want to rely on it.
I would argue that the user of the `ClusterClientFactory` should ensure that
the cluster-id was set, but I didn't want to remove the auto-generation in
`ClusterClientFactory#createClusterDescriptor` because god knows what it might
break.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
