Dennis-Mircea commented on code in PR #1195:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1195#discussion_r3870314214
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/kubeclient/decorators/FlinkConfMountDecorator.java:
##########
@@ -249,4 +251,20 @@ public String getFlinkConfFilename() {
public static boolean useStandardYamlConfig(FlinkVersion flinkVersion) {
return flinkVersion != null &&
flinkVersion.isEqualOrNewer(FlinkVersion.v2_0);
}
+
+ /**
+ * Re-parses Map-typed options that arrive already serialized to a String
(e.g. the autoscaler
+ * parallelism overrides stored in the CR spec) and sets them back as
typed values.
+ */
+ public static void normalizeMapTypedOptions(Configuration source,
Configuration target) {
+ if (!source.containsKey(PipelineOptions.PARALLELISM_OVERRIDES.key())) {
Review Comment:
The autoscaler sets it in legacy format based on my previous argument of
having the operator downgrades covered. This method makes the deployed
configuration properties, which are serialized differently by the standard YAML
parser and owned by the operator, independent of what the stored value looks
like, whether it came from a manual user edit, or if it got tampered. This is
more like a security safeguard.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/tuning/ConfigChanges.java:
##########
@@ -39,7 +39,8 @@ public class ConfigChanges {
@Getter private final Set<String> removals = new HashSet<>();
public <T> ConfigChanges addOverride(ConfigOption<T> configOption, T
value) {
- overrides.put(configOption.key(),
ConfigurationUtils.convertValue(value, String.class));
+ overrides.put(
+ configOption.key(), ConfigurationUtils.convertValue(value,
String.class, false));
Review Comment:
Good question. I'll try to explain a bit in detail about this one.
I decided to go this way mainly because users may need to downgrade the
operator from 1.16.0 back to 1.15.0 (maybe because something did not go as
expected). If this is not set to false and we go with the standard YAML parser,
which is the default in 1.16.0, then once downgraded to 1.15.0, the legacy YAML
parser will fail to read the standard YAML format.
The above argument is the reason for setting the flag to false everywhere in
this PR. This place concretely is not problematic, because it only handles
memory tuning keys, which are sizes and fractions, not Lists or Maps, which are
the types that serialize differently under the standard YAML format. I set it
to false here as well just to align with the other places and to avoid
confusion about the overall approach.
From the implementation point of view, there is also a limitation: the
`flink-autoscaler` module does not depend on the `flink-kubernetes-operator`
modules, so `FlinkVersion` is not visible at this layer, and the standalone
autoscaler has no such notion at all.
I think it is safer to proceed this way for now, for at least the next
couple of releases. Once support for Flink below 2.0 is dropped and downgrades
to 1.15.0 are no longer a concern, we can move to standard YAML everywhere and
drop this. What do you say?
--
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]
