Add Broker config `druid.broker.segment.watchRealtimeNodes` by kfaraz · Pull Request #11732 · apache/druid

kfaraz · 2021-09-22T09:13:09Z

Description

The new config is an extension of the concept of "watchedTiers" where
the Broker can choose to add the info of only the specified tiers to its timeline.
Similarly, with this config, Broker can choose to skip the realtime nodes and
thus it would query only Historical processes for any given segment.

Changes

Add new config druid.broker.segment.watchRealtimeNodes
Filter servers of type REALTIME in BrokerServerView based on whether realtime nodes are watched or not

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

abhishekagarwal87

LGTM. Thank you @kfaraz

techdocsmith

Tried to simply and clarify a little bit.

clintropolis · 2021-09-24T03:27:40Z

-      return true;
+      // Include realtime nodes only if they are watched
+      return segmentWatcherConfig.isWatchRealtimeNodes()
+          || metadataAndSegment.lhs.getType() != ServerType.REALTIME;


This should be looking for INDEXER_EXECUTOR, not sure REALTIME is actually used anymore (it was for old realtime nodes I think)

yeah, looks like usages were removed in #7915 and just exists now for ... reasons.

Thanks a lot for catching this! I can check for INDEXER_EXECUTOR here.

Considering that we are looking for INDEXER_EXECUTOR here, the nomenclature watchRealtimeNodes is also a little misleading now.

we should also add a warning in the code. does calling isSegmentReplicationTarget() makes more sense than checking the server type?

does calling isSegmentReplicationTarget() makes more sense than checking the server type?

Hmm, that one is kinda weird too because of ServerType.BRIDGE which uh.. predates me and Druid i think, so I'm not really sure its story.

maybe watchRealtimeTasks would be clearer?

watchRealtimeTasks works too.

Renamed config to watchRealtimeTasks. Fixed condition to check for ServerType.INDEXER_EXECUTOR instead of ServerType.REALTIME.

clintropolis · 2021-09-24T03:28:00Z

+  public boolean isWatchRealtimeNodes()
+  {
+    return watchRealtimeNodes;
+  }


I think my only real question about this PR is if this should maybe be watchedServerTypes or something and take a list of server types to watch instead of just a boolean

I was thinking the same thing but then I realized that only a few combinations are possible here:
(a) INDEXER_EXECUTOR and HISTORICAL, (b) only HISTORICAL (and maybe (c) only INDEXER_EXECUTOR).

As pointed out, REALTIME has been deprecated and not being used anywhere.

Could there be other queryable types in the future (or even now that I may have missed)?. If not, then I guess with the boolean, we just miss out on option (c) above.

Please let me know what you think.

if you have broadcast load rules, and segment cache configured, brokers can load segments as well. They currently have independent handling in BrokerServerView (https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/client/BrokerServerView.java#L226), but i think could in theory also be filtered here.

If we did do a list, the default value should probably be like, all the server types.

I'm not necessarily advocating for it to be a list.. just thinking out loud. A deny list might be better if we do go that route, or only filtering if the list isn't empty or something friendly.

watchedServerTypes seems too broad and technical. Trying to understand the use case behind such a config. e.g. what would a watchedServerTypes=Broker would mean?

so we would set something like druid.server.tier=_realtime on MM/indexers and then not include that in the watchedTiers? Is that what you are suggesting or something else?

yes, that exactly is what i was wondering if would work

it is worth trying. Hope it doesn't break something else 😄

@clintropolis - thought about this more and while playing with tier names is a workable approach, having a separate config to exclude real-time nodes makes config management easier. Like if I am trying to build an isolated broker-historical cohort, I can set watchRealtimeNodes = false on that cohort. The rest of the cluster requires no update.

set ignoreRealTime to true on that cohort

watchRealtimeNodes = false

…ker_config_watch_realtime

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

clintropolis

👍

Add config druid.broker.segment.watchRealtimeNodes

155dbb1

abhishekagarwal87 added Design Review Release Notes labels Sep 22, 2021

abhishekagarwal87 requested a review from techdocsmith September 22, 2021 09:18

abhishekagarwal87 approved these changes Sep 22, 2021

View reviewed changes

techdocsmith requested changes Sep 23, 2021

View reviewed changes

Comment thread docs/configuration/index.md Outdated

clintropolis requested changes Sep 24, 2021

View reviewed changes

kfaraz added 3 commits October 3, 2021 08:32

temp stuff

880c9e7

Rename field watchRealtimeNodes to watchRealtimeTasks

e609c19

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

7ff1a47

…ker_config_watch_realtime

kfaraz requested a review from abhishekagarwal87 October 21, 2021 08:11

kfaraz and others added 2 commits October 21, 2021 13:43

Update docs/configuration/index.md

fed5664

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

Rename config watchRealtimeNodes to watchRealtimeTasks

c2090fc

kfaraz requested review from clintropolis and techdocsmith October 21, 2021 08:15

kfaraz added 3 commits October 21, 2021 14:04

Fix changes made for testing

624f075

Remove test log lines

88f812c

Restore test BrokerServerViewTest.testIgnoredTiers()

c2eb1c1

clintropolis approved these changes Oct 22, 2021

View reviewed changes

abhishekagarwal87 approved these changes Oct 25, 2021

View reviewed changes

abhishekagarwal87 merged commit a22687e into apache:master Nov 2, 2021

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request Jun 22, 2022

[Draft] 0.23.0 Release notes #12510

Closed

kfaraz deleted the add_broker_config_watch_realtime branch September 30, 2022 16:07

Conversation

kfaraz commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Uh oh!

techdocsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kfaraz commented Sep 22, 2021 •

edited

Loading

kfaraz Sep 24, 2021 •

edited

Loading

kfaraz Sep 24, 2021 •

edited

Loading

clintropolis Sep 24, 2021 •

edited

Loading

abhishekagarwal87 Oct 8, 2021 •

edited

Loading

kfaraz Oct 8, 2021 •

edited

Loading