gianm commented on code in PR #19223:
URL: https://github.com/apache/druid/pull/19223#discussion_r3013684034
##########
indexing-service/src/main/java/org/apache/druid/indexing/compact/OverlordCompactionScheduler.java:
##########
@@ -264,6 +266,8 @@ public CompactionConfigValidationResult
validateCompactionConfig(DataSourceCompa
{
if (compactionConfig == null) {
return CompactionConfigValidationResult.failure("Cannot be null");
+ } else if (compactionConfig instanceof CascadingReindexingTemplate) {
Review Comment:
I think it'd be better to avoid the `instanceof` by putting the config in
control of its own validation, such as by adding an interface method on
`DataSourceCompactionConfig` like:
```
CompactionConfigValidationResult validate(ClusterCompactionConfig
clusterConfig)
```
Btw, I know this code was pre-existing, but the existing path uses
`getLatestClusterConfig()` to validate. Isn't that strange? It means that
whether a `CompactionSupervisorSpec` is readable or not will depend on the
current cluster configuration. If the cluster config changes then a
currently-running supervisor could stop having a valid spec. Also, validation
happens only on construction of the spec when it's deserialized, even if the
cluster configuration later changes in a way that makes a previously-valid spec
now-invalid.
Wouldn't it be better to validate when we run the spec in
`resetCompactionJobQueue`, rather than on spec deserialization?
--
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]
