kfaraz commented on code in PR #12615:
URL: https://github.com/apache/druid/pull/12615#discussion_r973585921
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -216,19 +216,29 @@ private static Set<String> parseJsonStringOrArray(Object
jsonStringOrArray)
}
}
- public static AtomicReference<CoordinatorDynamicConfig> watch(final
JacksonConfigManager configManager)
+ /**
+ * Setup a watch on the {@link CoordinatorDynamicConfig} in order to ensure
access to the latest stored version of the config.
+ *
+ * Note that the {@link CoordinatorDynamicConfig.Builder} class is used here
for serde because that allows clean setting of
+ * defaults for missing configuation keys. Missing configuration keys are
common in cases such as the addition of a
+ * new config key in a new Druid version.
+ *
+ * @param configManager {@link JacksonConfigManager } responsible for
managing persisted druid configuration content
+ * @return {@link AtomicReference} of {@link
CoordinatorDynamicConfig.Builder}
+ */
+ public static AtomicReference<CoordinatorDynamicConfig.Builder> watch(final
JacksonConfigManager configManager)
Review Comment:
The above javadoc could then be updated and moved to the `current()` method.
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -216,19 +216,29 @@ private static Set<String> parseJsonStringOrArray(Object
jsonStringOrArray)
}
}
- public static AtomicReference<CoordinatorDynamicConfig> watch(final
JacksonConfigManager configManager)
+ /**
+ * Setup a watch on the {@link CoordinatorDynamicConfig} in order to ensure
access to the latest stored version of the config.
+ *
+ * Note that the {@link CoordinatorDynamicConfig.Builder} class is used here
for serde because that allows clean setting of
+ * defaults for missing configuation keys. Missing configuration keys are
common in cases such as the addition of a
+ * new config key in a new Druid version.
+ *
+ * @param configManager {@link JacksonConfigManager } responsible for
managing persisted druid configuration content
+ * @return {@link AtomicReference} of {@link
CoordinatorDynamicConfig.Builder}
+ */
+ public static AtomicReference<CoordinatorDynamicConfig.Builder> watch(final
JacksonConfigManager configManager)
Review Comment:
I think we should also make this method private while we are at it as it is
not being used anywhere except this class itself. We also already have the
`current` method which is much more convenient. I can't think of a use case
where `watch()` would be needed.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
