tillrohrmann commented on a change in pull request #14220:
URL: https://github.com/apache/flink/pull/14220#discussion_r531453740
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/ClusterConfigurationInfo.java
##########
@@ -45,10 +59,19 @@ public static ClusterConfigurationInfo from(Configuration
config) {
final Map<String, String>
configurationWithHiddenSensitiveValues =
ConfigurationUtils.hideSensitiveValues(config.toMap());
for (Map.Entry<String, String> keyValuePair :
configurationWithHiddenSensitiveValues.entrySet()) {
- clusterConfig.add(new
ClusterConfigurationInfoEntry(keyValuePair.getKey(), keyValuePair.getValue()));
+
clusterConfig.add(createClusterConfigurationInfoEntry(keyValuePair.getKey(),
keyValuePair.getValue()));
}
return clusterConfig;
}
+ @VisibleForTesting
+ static ClusterConfigurationInfoEntry<?>
createClusterConfigurationInfoEntry(String key, String valueStr) {
+ if (MEMORY_OPTION_KEYS.contains(key)) {
+ return new ClusterConfigurationInfoEntry<>(key,
MemorySize.parse(valueStr).getBytes());
+ } else {
+ return new ClusterConfigurationInfoEntry<>(key,
valueStr);
+ }
Review comment:
It is used for displaying the configuration in the web UI, right?
Adding/enriching the memory configuration which are potentially derived from
some other values is fine in my opinion.
From a maintenance point of view I would also say that this change might
lead to confusion. Assuming time passes and somebody new has to change this
part. I am pretty sure that he will not understand why certain options are
reformatted and others not.
----------------------------------------------------------------
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:
[email protected]
