Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig #15705

kfaraz · 2024-01-17T11:38:20Z

Description

This is a follow up to #14695 to move configs from using skife to regular JsonConfigurator.

Advantages:

More structured configs that adhere to rest of the Druid code.
Classes that need only a small subset of configs need not be passed the entire DruidCoordinatorConfig object
Easier validation of configs done on service startup rather than when running duties.

Change summary

Add new config classes CoordinatorRunConfig, CoordinatorKillConfigs, CoordinatorPeriodConfig
Bind new config classes to property paths instead of DruidCoordinatorConfig
Make DruidCoordinatorConfig act as a container for the above configs
Add MetadataCleanupConfig to serve as a common config container for all metadata cleanups
Move validations from MetadataCleanupDuty to DruidCoordinatorConfig for cleaner code and to allow coordinator to fail fast on startup in case of invalid config values.
Disable zk-based segment loading option
All configs (except zk-based loading) retain their current paths and default values

Added configs

1. `CoordinatorRunConfig`

Path: druid.coordinator
Fields: period, startDelay
Description: Configs related to the running of the coordinator

2. `CoordinatorPeriodConfig`

Path: druid.coordinator.period
Fields: indexingPeriod, metadataStoreManagementPeriod
Description: Additional periods related to the operation of the coordinator

3. `CoordinatorKillConfigs`

Path: druid.coordinator.kill
Fields 1: audit, compaction, datasource, rules, supervisors, pendingSegments
Description: Each of the above fields is deserialized as a MetadataCleanupConfig which in-turn contains the following fields:

on: Whether cleanup is enabled
period: Cleanup period
durationToRetain: Duration of metadata to retain

Fields 2: on, period, durationToRetain, bufferPeriod, ignoreDurationToRetain, maxSegments
Description: These fields are related to cleanup of unused segments.

ZK-based segment loading disabled

ZK-based segment loading has been deprecated for several Druid releases
This PR is getting rid of the config druid.coordinator.loadqueuepeon.type, thus always using HttpLoadQueuePeon
The code for ZK-based segment loading is not being removed yet as it might be needed for testing purposes.

Release note

Zookeeper-based segment loading is being removed as it is known to have issues and has been deprecated for several releases. The recent improvements made to the Druid coordinator are also known to work much better with HTTP-based segment loading.

The following configs are being removed as they are not needed anymore:

druid.coordinator.load.timeout: Not needed as the default value of this parameter (15 minutes) is known to work well for all clusters
druid.coordinator.loadqueuepeon.type: Not needed as this value will always be http
druid.coordinator.curator.loadqueuepeon.numCallbackThreads: Not needed as zookeeper(curator)-based segment loading is not an option anymore

Auto-cleanup of compaction configs of inactive datasources is now enabled by default.

This PR has:

…ife_config

server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java

server/src/test/java/org/apache/druid/server/coordinator/duty/KillSupervisorsTest.java

abhishekrb19

Looks great! Checkpointing an initial review with some questions and suggestions.

server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java

abhishekrb19 · 2024-04-23T19:50:44Z

server/src/test/java/org/apache/druid/server/coordinator/loading/LoadQueuePeonTest.java

-            .withCoordinatorKillMaxSegments(10)
-            .withCoordinatorKillIgnoreDurationToRetain(false)
-            .build()
+        // 100ms is a great price to pay if it removes the flakeyness,


;)

Suggested change

// 100ms is a great price to pay if it removes the flakeyness,

// 100ms is a great price to pay if it removes the flakiness,

server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java

server/src/main/java/org/apache/druid/server/coordinator/duty/KillSupervisorsCustomDuty.java

abhishekrb19 · 2024-04-23T22:32:40Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillSupervisorsCustomDuty.java

    );
-    this.metadataSupervisorManager = metadataSupervisorManager;
    log.warn("This is only an example implementation of a custom duty and"


I suppose if the delegate is KillSupervisors, this log is not accurate anymore?
Also, curious, if this is meant to be an example implementation of a custom duty, should this code just reside in a separate tree in the repo?

Yeah, I actually wanted to get rid of this duty altogether and have a simpler DummyCustomCoordinatorDuty which just logged something in every run rather than doing something meaningful. It doesn't make sense to have a full fledged implementation and then advise users against using it.

What do you think?

server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java

server/src/main/java/org/apache/druid/server/coordinator/config/MetadataCleanupConfig.java

kfaraz · 2024-04-24T03:05:08Z

Thanks a lot for the review, @abhishekrb19 ! I have replied to your comments.

…ife_config

abhishekrb19

Thanks for the refactor @kfaraz! The configuration is so much easier to follow now. I've left a few non-blocking comments.

server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java

abhishekrb19 · 2024-04-28T23:57:05Z

server/src/main/java/org/apache/druid/server/coordinator/config/MetadataCleanupConfig.java

+
+public class MetadataCleanupConfig
+{
+  public static final MetadataCleanupConfig DEFAULT = new MetadataCleanupConfig(null, null, null);


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?

It doesn't actually conflict with the constructor. Since it is using the same constructor, it would have the same default values applied, i.e. true, 1 day, 90 days.

This is just used as short-hand in tests and in the class CoordinatorKillConfigs.

server/src/main/java/org/apache/druid/server/coordinator/config/KillUnusedSegmentsConfig.java

server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java

server/src/main/java/org/apache/druid/server/coordinator/loading/CuratorLoadQueuePeon.java

…ife_config

Follow up to #15705 Changes: - Remove references to ZK-based segment loading in the docs - Fix doc for existing config `druid.coordinator.loadqueuepeon.http.repeatDelay`

* Remove usage of skife from DruidCoordinatorConfig * Remove old config class * Address static checks * Fix tests * Remove unnecessary mocks * Fix config typos * Fix config condition * Fix test, spotbug check * Move validation to DruidCoordinatorConfig * Move DruidCoordinatorConfig to different package * Fix validation of killunusedconfig * Simplify and fix KillSupervisorsCustomDuty * Address review comments * Fix new tests * Add KillUnusedSchemasConfig * Remove KillUnusedSchemasConfig * Minor renames

* Remove usage of skife from DruidCoordinatorConfig (#15705) * Followup changes to 15817 (Segment schema publishing and polling) (#16368)

Background: ZK-based segment loading has been completely disabled in #15705 . ZK `servedSegmentsPath` has been deprecated since Druid 0.7.1, #1182 . This legacy path has been replaced by the `liveSegmentsPath` and is not used in the code anymore. Changes: - Never create ZK loadQueuePath as it is never used. - Never create ZK servedSegmentsPath as it is never used. - Do not create ZK liveSegmentsPath if announcement on ZK is disabled - Fix up tests

Remove usage of skife from DruidCoordinatorConfig

4f14ccf

kfaraz added the WIP label Jan 17, 2024

kfaraz added 2 commits January 17, 2024 17:09

Remove old config class

34e3bd4

Address static checks

7db4bc4

kfaraz removed the WIP label Jan 17, 2024

github-actions bot added WIP Area - Dependencies labels Jan 17, 2024

kfaraz requested a review from clintropolis January 17, 2024 12:31

kfaraz added 4 commits January 17, 2024 19:02

Fix tests

748d37b

Remove unnecessary mocks

550d0bb

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

da9c528

…ife_config

Fix config typos

a85ec53

kfaraz added Release Notes and removed WIP labels Jan 18, 2024

kfaraz added 4 commits February 9, 2024 11:19

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

611d940

…ife_config

Fix config condition

06b97cb

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

b0c7fa1

…ife_config

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

7f55e9a

…ife_config

kfaraz mentioned this pull request Apr 9, 2024

Set default KillUnusedSegments duty to coordinator's indexing period & killTaskSlotRatio to 0.1 #16247

Merged

6 tasks

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

e9a5e64

…ife_config

kfaraz requested a review from abhishekrb19 April 19, 2024 03:49

kfaraz added 2 commits April 19, 2024 10:17

Fix test, spotbug check

62f8920

Move validation to DruidCoordinatorConfig

0020446

kfaraz commented Apr 19, 2024

View reviewed changes

server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java Show resolved Hide resolved

kfaraz commented Apr 19, 2024

View reviewed changes

server/src/test/java/org/apache/druid/server/coordinator/duty/KillSupervisorsTest.java Show resolved Hide resolved

kfaraz added 3 commits April 19, 2024 10:44

Move DruidCoordinatorConfig to different package

6ce7c26

Fix validation of killunusedconfig

18e79a4

Simplify and fix KillSupervisorsCustomDuty

1c26bc1

kfaraz added the Refactoring label Apr 20, 2024

abhishekrb19 reviewed Apr 23, 2024

View reviewed changes

Address review comments

a2a928c

kfaraz added 2 commits April 25, 2024 07:38

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

0f18464

…ife_config

Fix new tests

47a9e4e

kfaraz requested a review from abhishekrb19 April 25, 2024 02:45

kfaraz added 2 commits April 25, 2024 08:52

Add KillUnusedSchemasConfig

032f2b7

Remove KillUnusedSchemasConfig

c10a5c9

abhishekrb19 approved these changes Apr 29, 2024

View reviewed changes

kfaraz added 3 commits April 29, 2024 07:58

Minor renames

9e79a07

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

65d774d

…ife_config

Merge branch 'master' of github.com:apache/druid into remove_coord_sk…

c7b04af

…ife_config

abhishekrb19 merged commit aa46314 into apache:master Apr 29, 2024
88 checks passed

kfaraz deleted the remove_coord_skife_config branch April 30, 2024 02:26

This was referenced Apr 30, 2024

Fix defaults of segment schema cleanup #16354

Merged

Docs: Remove references to Zk-based segment loading #16360

Merged

findingrish mentioned this pull request May 3, 2024

Backport 15705 & 16368 to Druid 30 #16384

Merged

kfaraz pushed a commit that referenced this pull request May 6, 2024

[Backport] 15705 & 16368 to Druid 30 (#16384)

6b29f7d

* Remove usage of skife from DruidCoordinatorConfig (#15705) * Followup changes to 15817 (Segment schema publishing and polling) (#16368)

kfaraz changed the title ~~Remove usage of skife from DruidCoordinatorConfig~~ Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig May 10, 2024

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

kfaraz mentioned this pull request Jul 30, 2024

Do not create ZK paths if not needed #16816

Merged

10 tasks

kfaraz added this to the 30.0.0 milestone Jul 30, 2024

soenkeliebau mentioned this pull request Nov 13, 2024

chore(tracking): Release SDP 24.11 stackabletech/issues#647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig #15705

Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig #15705

kfaraz commented Jan 17, 2024 •

edited

Loading

abhishekrb19 left a comment

abhishekrb19 Apr 23, 2024

abhishekrb19 Apr 23, 2024

kfaraz Apr 24, 2024 •

edited

Loading

kfaraz commented Apr 24, 2024

abhishekrb19 left a comment

abhishekrb19 Apr 28, 2024

kfaraz Apr 29, 2024

	// 100ms is a great price to pay if it removes the flakeyness,
	// 100ms is a great price to pay if it removes the flakiness,

Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig #15705

Disable ZK-based segment loading, remove usage of skife from DruidCoordinatorConfig #15705

Conversation

kfaraz commented Jan 17, 2024 • edited Loading

Description

Change summary

Added configs

1. CoordinatorRunConfig

2. CoordinatorPeriodConfig

3. CoordinatorKillConfigs

ZK-based segment loading disabled

Release note

abhishekrb19 left a comment

Choose a reason for hiding this comment

abhishekrb19 Apr 23, 2024

Choose a reason for hiding this comment

abhishekrb19 Apr 23, 2024

Choose a reason for hiding this comment

kfaraz Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

kfaraz commented Apr 24, 2024

abhishekrb19 left a comment

Choose a reason for hiding this comment

abhishekrb19 Apr 28, 2024

Choose a reason for hiding this comment

kfaraz Apr 29, 2024

Choose a reason for hiding this comment

kfaraz commented Jan 17, 2024 •

edited

Loading

1. `CoordinatorRunConfig`

2. `CoordinatorPeriodConfig`

3. `CoordinatorKillConfigs`

kfaraz Apr 24, 2024 •

edited

Loading