-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add changes to prevent Broker from querying cloned Historicals #17899
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.
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
Add changes to prevent Broker from querying cloned Historicals #17899
Conversation
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
Fixed
Show fixed
Hide fixed
server/src/main/java/org/apache/druid/server/DruidInternalDynamicConfigResource.java
Fixed
Show fixed
Hide fixed
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some initial thoughts.
server/src/main/java/org/apache/druid/client/DynamicConfigurationManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/DruidInternalDynamicConfigResource.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/metrics/TestDynamicConfigurationManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
Fixed
Show fixed
Hide fixed
server/src/main/java/org/apache/druid/client/selector/HistoricalFilter.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/query/QueryContexts.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/http/CoordinatorDynamicConfigTest.java
Fixed
Show fixed
Hide fixed
Will update documentation regarding the context parameter and APIs soon. |
...stage-query/src/main/java/org/apache/druid/msq/dart/controller/DartTableInputSpecSlicer.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/query/CloneQueryMode.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/http/CoordinatorResource.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/DruidInternalDynamicConfigResource.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/CloneStatusMetrics.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/CloneStatusManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigSyncer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigSyncer.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/selector/ServerSelector.java
Dismissed
Show dismissed
Hide dismissed
server/src/main/java/org/apache/druid/server/http/BrokerSyncStatus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/BrokerViewOfCoordinatorConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!!
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/client/ServerViewUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several improvements that are needed in this PR.
There are comments from @clintropolis too that need to be addressed.
But approving it for the time being to unblock work.
processing/src/main/java/org/apache/druid/query/CloneQueryMode.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/query/CloneQueryMode.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/ServerCloneStatus.java
Outdated
Show resolved
Hide resolved
{ | ||
private final String host; | ||
private final int port; | ||
private final long syncTimeInMs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this represents a timestamp since epoch, it is expected to be in millis (since that is the convention followed in the rest of Druid).
We can just call this lastSyncTime
or lastSyncTimestamp
.
private final long syncTimeInMs; | |
private final long syncTimeInMs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this represents a timestamp since epoch, it is expected to be in millis (since that is the convention followed in the rest of Druid).
Is there some dev documentation which mentions the above ?
IMHO units are better represented with the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unfortunately, it is not documented anywhere.
I agree that calling out the units removes the ambiguity, esp. when it comes to specifying durations.
(Side note: when writing configs for durations, we should either just call out the unit in the config name or accept a Duration
or Period
in the config value. We should probably document this somewhere as a Druid coding guideline.)
But I actually had other concerns with this field name.
syncTimeInMs
sounds more like the duration it took to finish a sync, rather than the last sync timestamp.
So we should definitely call it lastSyncTimestamp
.
For clarity, we can add the unit suffix and calling it lastSyncTimestampMillis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @adarshsanjeev , note for follow up
server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigSyncer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigSyncer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements, @adarshsanjeev .
I have left some minor suggestions that can be addressed in a follow up PR.
|`debug`| `false` | Flag indicating whether to enable debugging outputs for the query. When set to false, no additional logs will be produced (logs produced will be entirely dependent on your logging level). When set to true, the following addition logs will be produced:<br />- Log the stack trace of the exception (if any) produced by the query | | ||
|`setProcessingThreadNames`|`true`| Whether processing thread names will be set to `queryType_dataSource_intervals` while processing a query. This aids in interpreting thread dumps, and is on by default. Query overhead can be reduced slightly by setting this to `false`. This has a tiny effect in most scenarios, but can be meaningful in high-QPS, low-per-segment-processing-time scenarios. | | ||
|`sqlPlannerBloat`|`1000`|Calcite parameter which controls whether to merge two Project operators when inlining expressions causes complexity to increase. Implemented as a workaround to exception `There are not enough rules to produce a node with desired properties: convention=DRUID, sort=[]` thrown after rejecting the merge of two projects.| | ||
|`cloneQueryMode`|`excludeClones`| Indicates whether clone Historicals should be queried by brokers. Clone servers are created by the `cloneServers` Coordinator dynamic configuration. Possible values are `excludeClones`, `includeClones` and `clonesPreferred`. `excludeClones` means that clone Historicals are not queried by the broker. `clonesPreferred` indicates that when given a choice between the clone Historical and the original Historical which is being cloned, the broker chooses the clones; Historicals which are not involved in the cloning process will still be queried. `includeClones` means that broker queries any Historical without regarding clone status. This parameter only affects native queries; MSQ does not query Historicals directly, and Dart will not respect this context parameter.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For follow up: Use preferClones
instead of clonesPreferred
.
public static final boolean DEFAULT_ENABLE_JOIN_FILTER_PUSH_DOWN = true; | ||
public static final boolean DEFAULT_ENABLE_JOIN_FILTER_REWRITE = true; | ||
public static final boolean DEFAULT_ENABLE_JOIN_FILTER_REWRITE_VALUE_COLUMN_FILTERS = false; | ||
public static final CloneQueryMode DEFAULT_CLONE_QUERY_MODE = CloneQueryMode.EXCLUDECLONES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for follow-up: Should the default be exclude or include?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default should be exclude as we do not want clone servers to be queried by default. They might be running different versions of the code etc, which means that by default we do not want normal queries to be aware of these servers. In fact, should INCLUDE be removed? I don't know of a use case for them, other than ensuring that the query hits any possible server if available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see your point. The use case doesn't seem all that important, except maybe to ensure that segment remains available even when the source server flickers (but that is already handled by having 2 replicas).
In fact, the includeClones
mode might actually cause changes in query performance in a non-deterministic manner.
Let's get rid of it for now. We can add it back if there seems to be a real need for it.
// Don't remove either. | ||
return Set.of(); | ||
default: | ||
throw DruidException.defensive("Unexpected value: [%s]", cloneQueryMode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow up:
throw DruidException.defensive("Unexpected value: [%s]", cloneQueryMode); | |
throw DruidException.defensive("Unexpected value of cloneQueryMode[%s]", cloneQueryMode); |
{ | ||
private final String host; | ||
private final int port; | ||
private final long syncTimeInMs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unfortunately, it is not documented anywhere.
I agree that calling out the units removes the ambiguity, esp. when it comes to specifying durations.
(Side note: when writing configs for durations, we should either just call out the unit in the config name or accept a Duration
or Period
in the config value. We should probably document this somewhere as a Druid coding guideline.)
But I actually had other concerns with this field name.
syncTimeInMs
sounds more like the duration it took to finish a sync, rather than the last sync timestamp.
So we should definitely call it lastSyncTimestamp
.
For clarity, we can add the unit suffix and calling it lastSyncTimestampMillis
.
{ | ||
private final String host; | ||
private final int port; | ||
private final long syncTimeInMs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @adarshsanjeev , note for follow up
this.configManager = configManager; | ||
this.jsonMapper = jsonMapper; | ||
this.druidNodeDiscovery = druidNodeDiscoveryProvider; | ||
this.exec = Execs.scheduledSingleThreaded("CoordinatorDynamicConfigSyncer-%d"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for follow up: There should preferably be a lifecycle stop method which shuts down this executor, as it allows for nicer cleanup of the resources used by this class.
Proceeding with the merge as all the requested changes have been made. |
Add metrics for monitoring cloning and broker config syncing. These metrics are: - config/brokerSync/time - config/brokerSync/total/time - config/brokerSync/error Also addresses followup comments from #17899 including: - Fixing documentation of context parameter - Add a more graceful shutdown - Rename some fields for consistency
Description
This PR is a followup to #17863. This allows bringing up of copies of existing historicals. However, we might or might not want to query these historicals. We would not want to query historicals which are busy catching up to their source historical. On the other hand, once it has caught up, we might want to only query the clone, to see if it is performing as expected. This PR adds the logic for brokers to decide if they should query clones or not.
The coordinator dynamic configuration which contains the
cloneServers
configuration is synced to all brokers, by the Coordinator. This allows brokers to know which servers are clones, and their sources. This PR adds new internal Broker APIs to get and set the coordinator dynamic configuration.There is a new Query context parameter
cloneQueryMode
. Setting this parameter toexcludeClones
(default) causes brokers to not query any clone historicals. Setting this parameter topreferClones
causes brokers to only query the clones and not their source historicals. Setting this parameter toincludeClones
causes brokers to query all historicals without considering clone status.This parameter only affects native queries; MSQ does not query Historicals directly, and Dart will not respect this context parameter.
The PR also adds some new experimental APIs which return the current status of the servers undergoing cloning.
Fixed a few misc. issues I ran into while testing the PR:
APIs
Add the following new experimental Broker APIs:
/druid-internal/v1/config/coordinator
/druid-internal/v1/config/coordinator
Add the following new experimental Coordinator APIs:
/druid/coordinator/v1/config/syncedBrokers
/druid/coordinator/v1/config/cloneStatus
Release note
cloneQueryMode
. Setting this parameter toexcludeClones
(default) causes brokers to not query any clone historicals. Setting this parameter topreferClones
causes brokers to only query the clones and not their source historicals. Setting this parameter toincludeClones
causes brokers to query all historicals without considering clone status. This parameter only affects native queries; MSQ does not query Historicals directly, and Dart will not respect this context parameter./druid/coordinator/v1/cloneStatus
to get information about ongoing cloning operations./druid/coordinator/v1/brokerConfigurationStatus
which returns the broker sync status for coordinator dynamic configs.Benchmarks
Ran CachingClusteredClientBenchmark without and without the changes.
Without
New
This PR has: