mjsax commented on code in PR #13909:
URL: https://github.com/apache/kafka/pull/13909#discussion_r1381139811
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -1216,6 +1254,7 @@ public static class ConfigKey {
public final List<String> dependents;
public final Recommender recommender;
public final boolean internalConfig;
+ public final Object overwrittenValue;
Review Comment:
Can we find a better name? Or at least add a comment to explain what this is?
Given what we use `overwrittenValue` to generate HTML only, should it be
`String` type?
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -1238,6 +1277,31 @@ public ConfigKey(String name, Type type, Object
defaultValue, Validator validato
this.displayName = displayName;
this.recommender = recommender;
this.internalConfig = internalConfig;
+ this.overwrittenValue = null;
+ }
+
+ public ConfigKey(String name, Type type, Object defaultValue,
Validator validator,
Review Comment:
Do we need a new constructor? Might be simpler to just keep one, and pass in
`null` for the new parameter if not used?
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -155,6 +155,29 @@ public ConfigDef define(String name, Type type, Object
defaultValue, Validator v
return define(new ConfigKey(name, type, defaultValue, validator,
importance, documentation, group, orderInGroup, width, displayName, dependents,
recommender, false));
}
+ /**
+ * Define a new configuration
+ * @param name the name of the config parameter
+ * @param type the type of the config
+ * @param defaultValue the default value to use if this config isn't
present
+ * @param validator the validator to use in checking the
correctness of the config
+ * @param importance the importance of this config
+ * @param documentation the documentation string for the config
+ * @param group the group this config belongs to
+ * @param orderInGroup the order of this config in the group
+ * @param width the width of the config
+ * @param displayName the name suitable for display
+ * @param dependents the configurations that are dependents of this
configuration
+ * @param recommender the recommender provides valid values given
the parent configuration values
+ * @param overwrittenValue the overwritten value of this configuration
Review Comment:
> the overwritten value
Not every descriptive IMHO. Can we explain it better?
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -1265,6 +1329,8 @@ protected String getConfigValue(ConfigKey key, String
headerName) {
if (key.hasDefault()) {
if (key.defaultValue == null)
return "null";
+ if (key.overwrittenValue != null)
Review Comment:
Would this need to go before `if (key.defaultValue == null)` ? (For
`state.dir` it does not matter because it does have a default, but for general
purpose it seems better to change the order and make `overwrittenValue` the
winning parameter if used?)
##########
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java:
##########
@@ -820,6 +820,7 @@ public class StreamsConfig extends AbstractConfig {
.define(STATE_DIR_CONFIG,
Type.STRING,
System.getProperty("java.io.tmpdir") + File.separator +
"kafka-streams",
+ System.getProperty("kafka.streams.state.dir"),
Review Comment:
I think it might be simpler to just hardcode what we want to put into the
docs, ie, just pass in String `"${java.io.tmpdir}"`.
--
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]
