You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/17 14:46:08 UTC
[GitHub] [druid] kfaraz commented on a diff in pull request #12615: Modify process for deserializing DynamicCoordinatorConfig
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org