Add paramter to loadstatus API to compute underdeplication against cluster view by zachjsh · Pull Request #11056 · apache/druid

zachjsh · 2021-03-31T18:46:46Z

Description

This change adds a query parameter computeUsingClusterView to loadstatus apis
that if specified have the coordinator compute undereplication for segments based
on the number of services available within cluster that the segment can be replicated
on, instead of the configured replication count configured in load rule. This query
parameter is only available when also requesting full loadstatus. A default load rule is
created in all clusters that specified that all segments should be replicated 2 times. As
replicas are forced to be on separate nodes in the cluster, this causes the loadstatus api
to report that there are under-replicated segments when there is only 1 data server in the
cluster. In this case, calling loadstatus api without this new query parameter will always
result in a response indicating under-replication of segments

loadstatus api returns 503 if the cluster object isnt yet initialized. This should only happen
during the period of time before the coordinator run loop has executed at least once, as this
object is set during every instantiation of the coordinator run loop.

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.

…uster view This change adds a query parameter `computeUsingClusterView` to loadstatus apis that if specified have the coordinator compute undereplication for segments based on the number of services available within cluster that the segment can be replicated on, instead of the configured replication count configured in load rule. A default load rule is created in all clusters that specified that all segments should be replicated 2 times. As replicas are forced to be on separate nodes in the cluster, this causes the loadstatus api to report that there are under-replicated segments when there is only 1 data server in the cluster. In this case, calling loadstatus api without this new query parameter will always result in a response indicating under-replication of segments

jon-wei · 2021-04-01T01:35:31Z

  }

+  @Override
+  public void updateUnderReplicatedWithClusterView(


Can you add an implementation for BroadcastDistributionRule as well? I think the implementation there could just call updateUnderReplicated and ignore the cluster parameter, since it's already looking at the available servers in the cluster.

jon-wei · 2021-04-01T01:37:57Z

+          break;
+        }
+
+        if (computeUsingClusterView && (rule instanceof LoadRule)) {


The (rule instanceof LoadRule) should be removed, since BroadcastDistributionRule can also load segments (it also returns true for canLoadSegments(), which is already checked before this)

Changed accordingly. Before though, for broadcast rule, it was falling through to else branch and effectively doing same thing. Thought it was weird to add implementation for Broadcast rule that didn't use the parameter, but I guess not a big deal.

Cool, thanks, I thought it would be cleaner to have the new method be supported by all segment-loading rules, vs. the behavior specific to just LoadRule

jon-wei · 2021-04-01T01:39:54Z

  {
    DataSourcesResource dataSourcesResource = new DataSourcesResource(inventoryView, segmentsMetadataManager, null, null, null, null);
-    Response response = dataSourcesResource.getDatasourceLoadstatus("datasource1", null, null, null, null);
+    Response response = dataSourcesResource.getDatasourceLoadstatus("datasource1", null, null, null, null, null);


Can you add tests that use the new parameter?

Added test.

…ster-view

zachjsh · 2021-04-01T21:45:29Z

@techdocsmith can you take a look at the change to docs.

techdocsmith

That last sentence was complicated. I am not sure if I parsed it out correctly, but mabye my suggestions can help lead to some clarification.

Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com>

techdocsmith

Some edits to the new bit and found some typos in the previous bit. I am still not 100% sure about this bit:

Or the number of replicas for the segment in each tier where it is configured to be replicated equals the available nodes of a service type that are currently allowed to load the segment in the tier.

I feel like it needs to be simplified or broken down somehow.

Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com>

zachjsh requested review from clintropolis and jon-wei March 31, 2021 18:47

* fix exception mapper

e94750c

clintropolis added Area - Operations Area - Segment Balancing/Coordination labels Mar 31, 2021

jon-wei reviewed Apr 1, 2021

View reviewed changes

zachjsh added 3 commits April 1, 2021 17:18

* Address review comments

e8ffad1

Merge remote-tracking branch 'apache/master' into load-status-use-clu…

8ebf4bb

…ster-view

* update external API docs

3c7c917

zachjsh requested a review from jon-wei April 1, 2021 21:36

techdocsmith requested changes Apr 1, 2021

View reviewed changes

Apply suggestions from code review

dd53000

Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com>

jon-wei approved these changes Apr 1, 2021

View reviewed changes

zachjsh added 2 commits April 1, 2021 20:09

* update more external docs

02dd1e6

* update javadoc

5230f8b

techdocsmith requested changes Apr 2, 2021

View reviewed changes

Comment thread docs/operations/api-reference.md Outdated

Comment thread docs/operations/api-reference.md Outdated

Comment thread docs/operations/api-reference.md Outdated

Comment thread docs/operations/api-reference.md Outdated

Comment thread docs/operations/api-reference.md Outdated

Apply suggestions from code review

7e049f8

Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com>

zachjsh closed this Apr 2, 2021

zachjsh reopened this Apr 2, 2021

zachjsh merged commit 8cf1e83 into apache:master Apr 5, 2021

zachjsh deleted the load-status-use-cluster-view branch April 5, 2021 04:02

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

Conversation

zachjsh commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

jon-wei Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

jon-wei Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

jon-wei Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

jon-wei Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh commented Apr 1, 2021

Uh oh!

techdocsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

techdocsmith left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zachjsh commented Mar 31, 2021 •

edited

Loading

techdocsmith left a comment •

edited

Loading