clintropolis commented on code in PR #17776:
URL: https://github.com/apache/druid/pull/17776#discussion_r1980325901
##########
server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java:
##########
@@ -58,20 +65,22 @@ public BrokerParallelMergeConfig(
@JsonProperty("targetRunTimeMillis") @Nullable Integer
targetRunTimeMillis,
@JsonProperty("initialYieldNumRows") @Nullable Integer
initialYieldNumRows,
@JsonProperty("smallBatchNumRows") @Nullable Integer smallBatchNumRows,
- @JacksonInject LegacyBrokerParallelMergeConfig oldConfig
+ @JsonProperty("task") @Nullable LegacyBrokerTaskMergeConfig
oldTaskConfig,
Review Comment:
I wonder if we could do this a bit simpler and not need to retain the config
types and wire them up, since any of these configs being set would now be
invalid. The rest of the initialization would be a lot more pleasant to look at
if it didn't have to check old config everywhere.
The first way could be to use
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java#L121
which was introduced relatively recently and just add the error messaging
here. I haven't checked to confirm this works, but I believe they would be in
the properties even if a json config wasn't wired up, so we could just add
checking for the deprecated configs here.
Alternatively, we could probably accept the old configs as `Map<String,
Object>` and just check the keys of the maps to provide error messages for all
of the wrong configs.
--
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]
