Skip to content
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

Jump to bottom

Move segment update APIs from Coordinator to Overlord #17545

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Move segment update APIs from Coordinator to Overlord #17545

wants to merge 11 commits into from

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Dec 6, 2024 •
edited
Loading

Description

#17336 proposes adding a cache of segments on the leader Overlord to speed up segment allocation and other segment actions. But any caching mechanism can be efficient only if the leader Overlord is the source of truth and all segment updates happen through it.

This PR tries to move the Coordinator APIs which update segments to the Overlord instead.

Changes

Main classes to review

  • DataSourcesResource
  • OverlordDataSourcesResources
  • OverlordClient and OverlordClientImpl

Summary of changes

  • Add OverlordDataSourcesResource with APIs to mark segments used/unused
  • Add corresponding methods to OverlordClient
  • Deprecate Coordinator APIs to update segments
  • Use OverlordClient in DataSourcesResource so that Coordinator APIs internally
    call the corresponding Overlord APIs
  • If the API call fails, fall back to updating the metadata store directly
  • Audit these actions only on the Overlord

Other minor changes

  • Do not perform null check on OverlordClient on the coordinator side DataSourcesResource.
    OverlordClient is always non-null in production.
  • Add new tests, fix existing ones
  • Complete the implementation of TestSegmentsMetadataManager

New Overlord APIs

  • Mark all segments of a datasource as unused:
    POST /druid/indexer/v1/datasources/{dataSourceName}
  • Mark all (non-overshadowed) segments of a datasource as used:
    DELETE /druid/indexer/v1/datasources/{dataSourceName}
  • Mark multiple segments as used
    POST /druid/indexer/v1/datasources/{dataSourceName}/markUsed
  • Mark multiple (non-overshadowed) segments as unused
    POST /druid/indexer/v1/datasources/{dataSourceName}/markUnused
  • Mark a single segment as used:
    POST /druid/indexer/v1/datasources/{dataSourceName}/segments/{segmentId}
  • Mark a single segment as unused:
    DELETE /druid/indexer/v1/datasources/{dataSourceName}/segments/{segmentId}

Sample response

{
    "numUpdatedSegments": 3
}

Rolling upgrades

During a rolling upgrade, if the leader Coordinator gets upgraded before the leader Overlord,
it might try to call new Overlord APIs but that would throw a 404 NOT FOUND error.

In this case, the Coordinator falls back to the original method of updating the metadata store directly.

Pending items

  • Cluster testing

Changes not in this PR

  • Document updates
  • Web console changes to use new APIs

Release note

Add new Overlord APIs to mark non-overshadowed segments as used

  • POST /druid/indexer/v1/datasources/{dataSourceName}
  • POST /druid/indexer/v1/datasources/{dataSourceName}/markUsed
  • POST /druid/indexer/v1/datasources/{dataSourceName}/segments/{segmentId}

Add new Overlord APIs to mark segments as unused:

  • POST /druid/indexer/v1/datasources/{dataSourceName}/markUnused
  • DELETE /druid/indexer/v1/datasources/{dataSourceName}/segments/{segmentId}
  • DELETE /druid/indexer/v1/datasources/{dataSourceName}

The corresponding Coordinator APIs are being deprecated and will be removed in a future release:

  • POST /druid/coordinator/v1/datasources/{dataSourceName}
  • POST /druid/coordinator/v1/datasources/{dataSourceName}/markUsed
  • POST /druid/coordinator/v1/datasources/{dataSourceName}/markUnused
  • POST /druid/coordinator/v1/datasources/{dataSourceName}/segments/{segmentId}
  • DELETE /druid/coordinator/v1/datasources/{dataSourceName}/segments/{segmentId}
  • DELETE /druid/coordinator/v1/datasources/{dataSourceName}

Until they are removed, these Coordinator APIs will internally invoke respective Overlord APIs.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • 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.

@github-actions github-actions bot added Area - Batch Ingestion Area - Querying Area - Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Dec 6, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 6, 2024
View reviewed changes
...rvice/src/main/java/org/apache/druid/indexing/overlord/http/OverlordDataSourcesResource.java
@ResourceFilters(DatasourceResourceFilter.class)
public Response markAllNonOvershadowedSegmentsAsUsed(
@PathParam("dataSourceName") final String dataSourceName,
@Context HttpServletRequest req

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'req' is never used.
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java Outdated
Comment on lines 641 to 643
return Response.status(Response.Status.BAD_REQUEST).entity(
org.apache.druid.java.util.common.StringUtils.format("Could not parse Segment ID[%s]", segmentIdString)
).build();

Check warning

Code scanning / CodeQL

Cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
Loading
.
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java Outdated
Comment on lines 672 to 674
return Response.status(Response.Status.BAD_REQUEST).entity(
org.apache.druid.java.util.common.StringUtils.format("Could not parse Segment ID[%s]", segmentIdString)
).build();

Check warning

Code scanning / CodeQL

Cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
Loading
.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
DataSourcesResource dataSourcesResource = createResource();
prepareRequestForAudit();
Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", payload, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, payload, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
DataSourcesResource dataSourcesResource = createResource();
prepareRequestForAudit();
Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", payload, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, segmentFilter, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
DataSourcesResource dataSourcesResource = createResource();
prepareRequestForAudit();

Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", payload, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, segmentFilter, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
DataSourcesResource dataSourcesResource = createResource();
prepareRequestForAudit();

Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", payload, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, segmentFilter, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
@@ -1370,7 +1399,7 @@
{
DataSourcesResource dataSourcesResource = createResource();

Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", null, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, null, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java

Response response = dataSourcesResource.markSegmentsAsUnused("datasource1", payload, request);
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, payload, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
EasyMock.replay(overlordClient);

DataSourcesResource dataSourcesResource = createResource();
Response response = dataSourcesResource.markSegmentsAsUnused(TestDataSource.WIKI, segmentFilter, request);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentsAsUnused
Loading
should be avoided because it has been deprecated.
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 7, 2024
View reviewed changes
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
Comment on lines +616 to +621
() -> dataSourcesResource.markAsUnusedAllSegmentsOrKillUnusedSegmentsInInterval(
"datasource",
"true",
"???",
request
)

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markAsUnusedAllSegmentsOrKillUnusedSegmentsInInterval
Loading
should be avoided because it has been deprecated.
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 11, 2024
View reviewed changes
...rvice/src/main/java/org/apache/druid/indexing/overlord/http/OverlordDataSourcesResource.java
Comment on lines +237 to +239
return Response.status(Response.Status.BAD_REQUEST).entity(
StringUtils.format("Could not parse Segment ID[%s] for DataSource[%s]", segmentIdString, dataSourceName)
).build();

Check warning

Code scanning / CodeQL

Cross-site scripting Medium

Cross-site scripting vulnerability due to a
user-provided value
Loading
.
Cross-site scripting vulnerability due to a
user-provided value
Loading
.
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java

DataSourcesResource dataSourcesResource = createResource();
final StringFullResponseHolder responseHolder = new StringFullResponseHolder(

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
DataSourcesResource.markSegmentAsUsed
Loading
should be avoided because it has been deprecated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AmatyaAvadhanula AmatyaAvadhanula Awaiting requested review from AmatyaAvadhanula

Assignees
No one assigned
Labels
Area - Batch Ingestion Area - Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying Design Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kfaraz @cryptoe