Dennis-Mircea commented on code in PR #1229:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1229#discussion_r4371111585
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -37,8 +43,25 @@ public class AutoScalerOptions {
public static final String CUSTOM_EVALUATOR_CONF_PREFIX =
"metrics.custom-evaluator.";
public static final String SCALING_CUSTOM_EXECUTOR_CONF_PREFIX =
"scaling.custom-executor.";
public static final String SCALING_ALIGNMENT_MODE_CONF_PREFIX =
+ "scaling.parallelism-alignment.modes.";
+
+ /**
+ * Template prefix of the custom alignment modes before FLINK-40873. It is
a strict prefix of
+ * {@link #ALIGNMENT_MODE}, which breaks converting a flat config map to
yaml. Keys under it are
+ * still read, but new configurations should use {@link
#SCALING_ALIGNMENT_MODE_CONF_PREFIX}.
+ */
Review Comment:
Too verbose. This Javadoc is not needed. Also, the properties prefixed with
this should be marked with `@Deprecated` so they are not displayed in the
documentation.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -690,31 +729,57 @@ public static UnmodifiableConfiguration
overlayConfiguration(
* Builds a fresh {@link Configuration} exposing the per-instance options
configured under
* {@code job.autoscaler.<confPrefix><name>.} (canonical) and {@code
* kubernetes.operator.job.autoscaler.<confPrefix><name>.} (legacy), with
that prefix stripped.
- * The legacy prefix is applied first so that, on overlap, values written
under the canonical
- * prefix take precedence, matching the semantics of {@link
+ * When several prefixes are given they are listed newest first and
applied oldest first, so
+ * that on overlap the newest canonical keys take precedence, matching the
semantics of {@link
* org.apache.flink.configuration.ConfigOption#withFallbackKeys} used
elsewhere in this class.
*/
private static Configuration prefixStrippedConfiguration(
- Configuration configuration, String confPrefix, String name) {
- String canonicalPrefix = AUTOSCALER_CONF_PREFIX + confPrefix + name +
".";
- String legacyPrefix = OLD_K8S_OP_CONF_PREFIX + canonicalPrefix;
- Configuration merged = new Configuration();
- configuration
- .toMap()
- .forEach(
- (k, v) -> {
- if (k.startsWith(legacyPrefix)) {
-
merged.setString(k.substring(legacyPrefix.length()), v);
- }
- });
- configuration
- .toMap()
- .forEach(
- (k, v) -> {
- if (k.startsWith(canonicalPrefix)) {
-
merged.setString(k.substring(canonicalPrefix.length()), v);
- }
- });
+ Configuration configuration, String name, String...
confPrefixesNewestFirst) {
+ final List<String> prefixesByDescendingPrecedence = new ArrayList<>();
+ for (String confPrefix : confPrefixesNewestFirst) {
+ final String canonicalPrefix = AUTOSCALER_CONF_PREFIX + confPrefix
+ name + ".";
+ prefixesByDescendingPrecedence.add(canonicalPrefix);
+ prefixesByDescendingPrecedence.add(OLD_K8S_OP_CONF_PREFIX +
canonicalPrefix);
+ }
+
+ final Map<String, String> flatConfiguration = configuration.toMap();
+ final Configuration merged = new Configuration();
+ for (int i = prefixesByDescendingPrecedence.size() - 1; i >= 0; i--) {
+ final String prefix = prefixesByDescendingPrecedence.get(i);
+ flatConfiguration.forEach(
+ (k, v) -> {
+ if (k.startsWith(prefix)) {
+ merged.setString(k.substring(prefix.length()), v);
+ }
+ });
+ }
return merged;
}
+
+ /**
+ * The deprecated {@code alignment.mode} template prefix is still read for
configurations
+ * written before FLINK-40873. Custom alignment modes are resolved per
vertex and per scaling
+ * run, so the configuration is only scanned once per JVM instead of on
every lookup.
+ */
+ private static void
warnIfDeprecatedAlignmentModePrefixIsUsed(Configuration configuration) {
Review Comment:
Not a necessary method/logic...
##########
docs/content.zh/docs/deployment/plugins.md:
##########
@@ -381,6 +381,8 @@ The autoscaler aligns a vertex's computed target
parallelism to the number of ke
All `job.autoscaler.*` keys related to custom alignment modes also support the
legacy `kubernetes.operator.`-prefixed form as a fallback (for example,
`kubernetes.operator.job.autoscaler.scaling.parallelism-alignment.mode`). The
canonical key takes precedence on overlap.
+The per-mode template keys were renamed to
`job.autoscaler.scaling.parallelism-alignment.modes.<name>.*`; keys under the
older `job.autoscaler.scaling.parallelism-alignment.mode.<name>.` prefix are
still read and log a deprecation warning.
Review Comment:
Not a necessary doc paragraph. We don't need to mention that. It can be
deleted.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -37,8 +43,25 @@ public class AutoScalerOptions {
public static final String CUSTOM_EVALUATOR_CONF_PREFIX =
"metrics.custom-evaluator.";
public static final String SCALING_CUSTOM_EXECUTOR_CONF_PREFIX =
"scaling.custom-executor.";
public static final String SCALING_ALIGNMENT_MODE_CONF_PREFIX =
+ "scaling.parallelism-alignment.modes.";
+
+ /**
+ * Template prefix of the custom alignment modes before FLINK-40873. It is
a strict prefix of
+ * {@link #ALIGNMENT_MODE}, which breaks converting a flat config map to
yaml. Keys under it are
+ * still read, but new configurations should use {@link
#SCALING_ALIGNMENT_MODE_CONF_PREFIX}.
+ */
+ public static final String DEPRECATED_SCALING_ALIGNMENT_MODE_CONF_PREFIX =
"scaling.parallelism-alignment.mode.";
+ private static final Logger LOG =
LoggerFactory.getLogger(AutoScalerOptions.class);
Review Comment:
This class should not have any logging mechanism.. It is just a plain class
for usual properties, and the static methods that are part of this class are
really lightweight.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -37,8 +43,25 @@ public class AutoScalerOptions {
public static final String CUSTOM_EVALUATOR_CONF_PREFIX =
"metrics.custom-evaluator.";
public static final String SCALING_CUSTOM_EXECUTOR_CONF_PREFIX =
"scaling.custom-executor.";
public static final String SCALING_ALIGNMENT_MODE_CONF_PREFIX =
+ "scaling.parallelism-alignment.modes.";
+
+ /**
+ * Template prefix of the custom alignment modes before FLINK-40873. It is
a strict prefix of
+ * {@link #ALIGNMENT_MODE}, which breaks converting a flat config map to
yaml. Keys under it are
+ * still read, but new configurations should use {@link
#SCALING_ALIGNMENT_MODE_CONF_PREFIX}.
+ */
+ public static final String DEPRECATED_SCALING_ALIGNMENT_MODE_CONF_PREFIX =
"scaling.parallelism-alignment.mode.";
+ private static final Logger LOG =
LoggerFactory.getLogger(AutoScalerOptions.class);
+
+ /**
+ * Guards {@link #warnIfDeprecatedAlignmentModePrefixIsUsed}, which has to
scan the whole
+ * configuration, to a single check.
+ */
+ private static final AtomicBoolean
DEPRECATED_ALIGNMENT_MODE_PREFIX_CHECKED =
Review Comment:
We should not display any logs or other logic about the usage of deprecated
keys... We are not doing this anywhere for any other properties that were
deprecated..
##########
docs/content/docs/deployment/plugins.md:
##########
@@ -381,6 +381,8 @@ The autoscaler aligns a vertex's computed target
parallelism to the number of ke
All `job.autoscaler.*` keys related to custom alignment modes also support the
legacy `kubernetes.operator.`-prefixed form as a fallback (for example,
`kubernetes.operator.job.autoscaler.scaling.parallelism-alignment.mode`). The
canonical key takes precedence on overlap.
+The per-mode template keys were renamed to
`job.autoscaler.scaling.parallelism-alignment.modes.<name>.*`; keys under the
older `job.autoscaler.scaling.parallelism-alignment.mode.<name>.` prefix are
still read and log a deprecation warning.
Review Comment:
Same here.
--
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]
