abhishekrb19 commented on code in PR #15705: URL: https://github.com/apache/druid/pull/15705#discussion_r1582436881
########## server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class CoordinatorKillConfigs +{ + public static CoordinatorKillConfigs STANDARD + = new CoordinatorKillConfigs(null, null, null, null, null, null, null, null, null, null, null, null, null); + + @JsonProperty("supervisor") + private final MetadataCleanupConfig supervisors; + + @JsonProperty("audit") + private final MetadataCleanupConfig audit; + + @JsonProperty("datasource") + private final MetadataCleanupConfig datasource; + + @JsonProperty("rule") + private final MetadataCleanupConfig rules; Review Comment: Same here. ########## server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class CoordinatorKillConfigs +{ + public static CoordinatorKillConfigs STANDARD + = new CoordinatorKillConfigs(null, null, null, null, null, null, null, null, null, null, null, null, null); + + @JsonProperty("supervisor") + private final MetadataCleanupConfig supervisors; Review Comment: Should we call the variable `supervisor` to align with the json annotation? Alternatively, if the plural form reads better, consider renaming datasource to datasources for consistency. ########## server/src/main/java/org/apache/druid/server/coordinator/loading/CuratorLoadQueuePeon.java: ########## @@ -72,7 +72,7 @@ public class CuratorLoadQueuePeon implements LoadQueuePeon * with loading or dropping segments. */ private final ExecutorService callBackExecutor; - private final DruidCoordinatorConfig config; + private final Duration loadTimeout; private final AtomicLong queuedSize = new AtomicLong(0); Review Comment: This class along with its test can be removed right? I see: > The code for ZK-based segment loading is not being removed yet as it might be needed for testing purposes. Are we referring to the unit tests? Is there a plan to migrate them to use the http strategy since it's the only option now? ########## server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class CoordinatorKillConfigs +{ + public static CoordinatorKillConfigs STANDARD + = new CoordinatorKillConfigs(null, null, null, null, null, null, null, null, null, null, null, null, null); Review Comment: This default constant appears to be used in test code. Maybe just inline the usage in the test? ########## server/src/main/java/org/apache/druid/server/coordinator/config/KillUnusedSegmentsConfig.java: ########## @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class KillUnusedSegmentsConfig extends MetadataCleanupConfig +{ + public static KillUnusedSegmentsConfig DEFAULT Review Comment: Unused and can be removed ########## server/src/main/java/org/apache/druid/server/coordinator/config/MetadataCleanupConfig.java: ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +import java.util.Objects; + +public class MetadataCleanupConfig +{ + public static final MetadataCleanupConfig DEFAULT = new MetadataCleanupConfig(null, null, null); Review Comment: Now that I look into this again, it looks like this DEFAULT conflcits with the actual defaults applied in the constructor. Do we need this static null config -- won't the json creator below actually handle the default values when it's invoked from the caller? ########## server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class CoordinatorKillConfigs +{ + public static CoordinatorKillConfigs STANDARD + = new CoordinatorKillConfigs(null, null, null, null, null, null, null, null, null, null, null, null); + + @JsonProperty("supervisor") + private final MetadataCleanupConfig supervisors; + + @JsonProperty("audit") + private final MetadataCleanupConfig audit; + + @JsonProperty("datasource") + private final MetadataCleanupConfig datasource; + + @JsonProperty("rule") + private final MetadataCleanupConfig rules; + + @JsonProperty("compaction") + private final MetadataCleanupConfig compaction; + + @JsonProperty("pendingSegments") + private final MetadataCleanupConfig pendingSegments; + + // Raw configs for killing unused segments + // These have been added as fields because KillUnusedSegmentsConfig is initialized lazily + @JsonProperty("on") + private final Boolean killUnusedEnabled; + + @JsonProperty("period") + private final Duration killUnusedPeriod; + + @JsonProperty("durationToRetain") + private final Duration killUnusedDurationToRetain; + + @JsonProperty("ignoreDurationToRetain") + private final Boolean killUnusedIgnoreDurationToRetain; + + @JsonProperty("bufferPeriod") + private final Duration killUnusedBufferPeriod; + + @JsonProperty("maxSegments") + private final Integer killUnusedMaxSegments; + + @JsonCreator + public CoordinatorKillConfigs( + @JsonProperty("pendingSegments") MetadataCleanupConfig pendingSegments, + @JsonProperty("supervisor") MetadataCleanupConfig supervisors, + @JsonProperty("audit") MetadataCleanupConfig audit, + @JsonProperty("datasource") MetadataCleanupConfig datasource, + @JsonProperty("rule") MetadataCleanupConfig rules, + @JsonProperty("compaction") MetadataCleanupConfig compaction, + // Configs for cleanup of unused segments Review Comment: Yeah, probably not worth the hassle ########## server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.joda.time.Duration; + +public class CoordinatorKillConfigs +{ + public static CoordinatorKillConfigs STANDARD + = new CoordinatorKillConfigs(null, null, null, null, null, null, null, null, null, null, null, null); + + @JsonProperty("supervisor") + private final MetadataCleanupConfig supervisors; + + @JsonProperty("audit") + private final MetadataCleanupConfig audit; + + @JsonProperty("datasource") + private final MetadataCleanupConfig datasource; + + @JsonProperty("rule") + private final MetadataCleanupConfig rules; + + @JsonProperty("compaction") + private final MetadataCleanupConfig compaction; + + @JsonProperty("pendingSegments") + private final MetadataCleanupConfig pendingSegments; + + // Raw configs for killing unused segments + // These have been added as fields because KillUnusedSegmentsConfig is initialized lazily Review Comment: I see, we don't need it then. -- 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]
