XComp commented on a change in pull request #17819: URL: https://github.com/apache/flink/pull/17819#discussion_r780084427
########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesLabel.java ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.utils; + +import org.apache.flink.kubernetes.configuration.KubernetesConfigOptions; + +import java.util.Arrays; + +import static org.apache.flink.util.Preconditions.checkArgument; +import static org.apache.flink.util.StringUtils.isNullOrWhitespaceOnly; + +/** Collection of kubernetes labels and helper methods. */ +public enum KubernetesLabel { + CONFIG_MAP_PREFIX("flink-config-", ""), + HADOOP_CONF_CONFIG_MAP_PREFIX("hadoop-config-", ""), + KERBEROS_KEYTAB_SECRET_PREFIX("kerberos-keytab-", ""), + KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX("kerberos-krb5conf-", ""), + POD_TEMPLATE_CONFIG_MAP_PREFIX("pod-template-", ""), + FLINK_REST_SERVICE_SUFFIX("", "-rest"); + + private final String prefix; + private final String suffix; + + KubernetesLabel(String prefix, String suffix) { + this.prefix = prefix; + this.suffix = suffix; + } + + private int getTotalLength() { + return prefix.length() + suffix.length(); + } + + @SuppressWarnings("OptionalGetWithoutIsPresent") + public static int getClusterIdMaxLength() { + int longestAffix = Review comment: nit: it's actually the `longestAffixLength` ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesLabel.java ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.utils; + +import org.apache.flink.kubernetes.configuration.KubernetesConfigOptions; + +import java.util.Arrays; + +import static org.apache.flink.util.Preconditions.checkArgument; +import static org.apache.flink.util.StringUtils.isNullOrWhitespaceOnly; + +/** Collection of kubernetes labels and helper methods. */ +public enum KubernetesLabel { + CONFIG_MAP_PREFIX("flink-config-", ""), + HADOOP_CONF_CONFIG_MAP_PREFIX("hadoop-config-", ""), + KERBEROS_KEYTAB_SECRET_PREFIX("kerberos-keytab-", ""), + KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX("kerberos-krb5conf-", ""), + POD_TEMPLATE_CONFIG_MAP_PREFIX("pod-template-", ""), + FLINK_REST_SERVICE_SUFFIX("", "-rest"); Review comment: nit: The `_PREFIX` and `_SUFFIX` extension became obsolete in the name. Whether there are prefixes or suffixes involved in the label generation shouldn't be reflected in the name anymore since it's an "implementation detail". We don't want to rename the enum when we (out of some reason which admittedly is theoretical) we decide to add some suffix or prefix where there was none before. So, essentially, each of the names make to total sense without the `_*FIX` extension (i.e. `FLINK_REST_SERVICE_SUFFIX` becomes `FLINK_REST_SERVICE`). ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/Constants.java ########## @@ -83,7 +77,7 @@ public static final String HEADLESS_SERVICE_CLUSTER_IP = "None"; - public static final int MAXIMUM_CHARACTERS_OF_CLUSTER_ID = 45; + public static final int KUBERNETES_LABEL_MAX_LENGTH = 63; Review comment: This can be moved into `KubernetesLabel` as a private constant: It's not used anywhere else. ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesLabel.java ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.utils; + +import org.apache.flink.kubernetes.configuration.KubernetesConfigOptions; + +import java.util.Arrays; + +import static org.apache.flink.util.Preconditions.checkArgument; +import static org.apache.flink.util.StringUtils.isNullOrWhitespaceOnly; + +/** Collection of kubernetes labels and helper methods. */ +public enum KubernetesLabel { + CONFIG_MAP_PREFIX("flink-config-", ""), + HADOOP_CONF_CONFIG_MAP_PREFIX("hadoop-config-", ""), + KERBEROS_KEYTAB_SECRET_PREFIX("kerberos-keytab-", ""), + KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX("kerberos-krb5conf-", ""), + POD_TEMPLATE_CONFIG_MAP_PREFIX("pod-template-", ""), + FLINK_REST_SERVICE_SUFFIX("", "-rest"); + + private final String prefix; + private final String suffix; + + KubernetesLabel(String prefix, String suffix) { + this.prefix = prefix; + this.suffix = suffix; + } + + private int getTotalLength() { + return prefix.length() + suffix.length(); + } + + @SuppressWarnings("OptionalGetWithoutIsPresent") + public static int getClusterIdMaxLength() { + int longestAffix = + Arrays.stream(KubernetesLabel.values()) + .map(KubernetesLabel::getTotalLength) + .max(Integer::compareTo) + .get(); + + return Constants.KUBERNETES_LABEL_MAX_LENGTH - longestAffix; + } + + /** + * Creates a kubernetes label containing the clusterId. + * + * @param clusterId The clusterId + * @param label The {@link KubernetesLabel} to apply + * @return a String containing the kubernetes label with the clusterId + */ + public static String create(String clusterId, KubernetesLabel label) { Review comment: nit: Is there a reason to make this a static method? Making it an instance method of `KubernesLabel` would minimize the method's signature and (imo) improve the method call, e.g.: `KubernetesLabel.FLINK_REST_SERVICE_SUFFIX.generateWith(clusterId);` instead of `KubernetesLabel.create(clusterId, KubernetesLabel.FLINK_REST_SERVICE_SUFFIX);` ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java ########## @@ -220,7 +221,9 @@ .withDescription( Description.builder() .text( - "The cluster-id, which should be no more than 45 characters, is used for identifying a unique Flink cluster. " + "The cluster-id, which should be no more than " + + KubernetesLabel.getClusterIdMaxLength() Review comment: We could us the `%s` placeholder here like it's done for the other variables to be more consistent. ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/utils/KubernetesLabel.java ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.utils; + +import org.apache.flink.kubernetes.configuration.KubernetesConfigOptions; + +import java.util.Arrays; + +import static org.apache.flink.util.Preconditions.checkArgument; +import static org.apache.flink.util.StringUtils.isNullOrWhitespaceOnly; + +/** Collection of kubernetes labels and helper methods. */ +public enum KubernetesLabel { + CONFIG_MAP_PREFIX("flink-config-", ""), + HADOOP_CONF_CONFIG_MAP_PREFIX("hadoop-config-", ""), + KERBEROS_KEYTAB_SECRET_PREFIX("kerberos-keytab-", ""), + KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX("kerberos-krb5conf-", ""), + POD_TEMPLATE_CONFIG_MAP_PREFIX("pod-template-", ""), + FLINK_REST_SERVICE_SUFFIX("", "-rest"); + + private final String prefix; + private final String suffix; + + KubernetesLabel(String prefix, String suffix) { + this.prefix = prefix; + this.suffix = suffix; + } + + private int getTotalLength() { + return prefix.length() + suffix.length(); + } + + @SuppressWarnings("OptionalGetWithoutIsPresent") + public static int getClusterIdMaxLength() { + int longestAffix = + Arrays.stream(KubernetesLabel.values()) + .map(KubernetesLabel::getTotalLength) + .max(Integer::compareTo) + .get(); Review comment: ```suggestion .orElseThrow(() -> new IllegalStateException("No enum value is present.")); ``` nit: you could remove this method's `@SuppressWarnings` annotation by stating the actual issue in the code. That makes the issue more explicit. ########## File path: flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/decorators/KerberosMountDecorator.java ########## @@ -198,10 +199,10 @@ public FlinkPod decorateFlinkPod(FlinkPod flinkPod) { } public static String getKerberosKeytabSecretName(String clusterId) { - return Constants.KERBEROS_KEYTAB_SECRET_PREFIX + clusterId; + return KubernetesLabel.create(clusterId, KubernetesLabel.KERBEROS_KEYTAB_SECRET_PREFIX); } - public static String getKerberosKrb5confConfigMapName(String clusterID) { - return Constants.KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX + clusterID; + public static String getKerberosKrb5confConfigMapName(String clusterId) { + return KubernetesLabel.create(clusterId, KubernetesLabel.KERBEROS_KEYTAB_SECRET_PREFIX); Review comment: ```suggestion return KubernetesLabel.create(clusterId, KubernetesLabel.KERBEROS_KRB5CONF_CONFIG_MAP_PREFIX); ``` -- 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
