Skip to content

Modify process for deserializing DynamicCoordinatorConfig #12615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
capistrant wants to merge 10 commits into from
Closed

Modify process for deserializing DynamicCoordinatorConfig #12615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c7ee569
Modify process for deserializing DynamicCoordinatorConfig
Jun 7, 2022
0b5e06c
small code cleanup and a javadoc on changed method
Jun 7, 2022
6d0bc49
update java doc
Jun 7, 2022
60b2410
edit javadoc
Jun 7, 2022
bdd327d
Merge branch 'master' into fixes-11161
Jun 22, 2022
783f79d
Merge branch 'master' into fixes-11161
Jun 28, 2022
7757c1e
Merge branch 'master' into fixes-11161
Jul 21, 2022
108aea4
Merge branch 'master' into fixes-11161
Aug 5, 2022
bec96a7
Merge branch 'master' into fixes-11161
Aug 11, 2022
2e38e2d
Merge branch 'master' into fixes-11161
Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above javadoc could then be updated and moved to the current() method.

{
return configManager.watch(
CoordinatorDynamicConfig.CONFIG_KEY,
CoordinatorDynamicConfig.class,
CoordinatorDynamicConfig.builder().build()
CoordinatorDynamicConfig.Builder.class,
CoordinatorDynamicConfig.builder()
);
}

@Nonnull
public static CoordinatorDynamicConfig current(final JacksonConfigManager configManager)
{
return Preconditions.checkNotNull(watch(configManager).get(), "Got null config from watcher?!");
return Preconditions.checkNotNull(watch(configManager).get().build(), "Got null config from watcher?!");
}

@JsonProperty("millisToWaitBeforeDeleting")
Expand Down
11 changes: 7 additions & 4 deletions server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public void setUp() throws Exception
EasyMock.anyObject(Class.class),
EasyMock.anyObject()
)
).andReturn(new AtomicReference(CoordinatorDynamicConfig.builder().build())).anyTimes();
).andReturn(new AtomicReference(CoordinatorDynamicConfig.builder())).anyTimes();
EasyMock.expect(
configManager.watch(
EasyMock.eq(CoordinatorCompactionConfig.CONFIG_KEY),
Expand Down Expand Up @@ -715,17 +715,20 @@ public void testBalancerThreadNumber()
EasyMock.expect(dynamicConfig.getBalancerComputeThreads()).andReturn(5).times(2);
EasyMock.expect(dynamicConfig.getBalancerComputeThreads()).andReturn(10).once();

CoordinatorDynamicConfig.Builder dynamicConfigBuilder = EasyMock.createNiceMock(CoordinatorDynamicConfig.Builder.class);
EasyMock.expect(dynamicConfigBuilder.build()).andReturn(dynamicConfig).anyTimes();

JacksonConfigManager configManager = EasyMock.createNiceMock(JacksonConfigManager.class);
EasyMock.expect(
configManager.watch(
EasyMock.eq(CoordinatorDynamicConfig.CONFIG_KEY),
EasyMock.anyObject(Class.class),
EasyMock.anyObject()
)
).andReturn(new AtomicReference(dynamicConfig)).anyTimes();
).andReturn(new AtomicReference(dynamicConfigBuilder)).anyTimes();

ScheduledExecutorFactory scheduledExecutorFactory = EasyMock.createNiceMock(ScheduledExecutorFactory.class);
EasyMock.replay(configManager, dynamicConfig, scheduledExecutorFactory);
EasyMock.replay(configManager, dynamicConfig, scheduledExecutorFactory, dynamicConfigBuilder);

DruidCoordinator c = new DruidCoordinator(
druidCoordinatorConfig,
Expand Down Expand Up @@ -998,7 +1001,7 @@ public void testCoordinatorCustomDutyGroupsRunAsExpected() throws Exception
EasyMock.anyObject(Class.class),
EasyMock.anyObject()
)
).andReturn(new AtomicReference(CoordinatorDynamicConfig.builder().build())).anyTimes();
).andReturn(new AtomicReference(CoordinatorDynamicConfig.builder())).anyTimes();
EasyMock.expect(
configManager.watch(
EasyMock.eq(CoordinatorCompactionConfig.CONFIG_KEY),
Expand Down