Skip to content

Inject new task count calculation during the rollover #18860

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
Merged
kfaraz merged 21 commits into from
Jan 5, 2026
Merged

Inject new task count calculation during the rollover #18860

kfaraz merged 21 commits into from
Jan 5, 2026

Conversation

@Fly-Style
Copy link
Contributor

@Fly-Style Fly-Style commented Dec 19, 2025 •
edited
Loading

As a follow-up PR for #18819, the patch fixes temporal behaviour when scale down happens in the same manner, as scaleup, by injecting the new task count calculation logic during the task rollover time,

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.

kfaraz
kfaraz reviewed Dec 22, 2025
View reviewed changes
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up, @Fly-Style !
Left some initial drive-by comments. Will do a more thorough review later today/tomorrow.

...ests/src/test/java/org/apache/druid/testing/embedded/indexing/TaskScaleDownRolloverTest.java Outdated
@SuppressWarnings("resource")
@Test
@Timeout(300)
void test_scaleDownDuringTaskRollover()
Copy link
Contributor

@kfaraz kfaraz Dec 22, 2025

Choose a reason for hiding this comment

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

Can this test not be moved to the CostBasedAutoScalerIntegrationTest itself?

Copy link
Contributor Author

@Fly-Style Fly-Style Dec 22, 2025

Choose a reason for hiding this comment

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

I decided to make it separate, because we're testing abstract autoscaler actions during the task rollover, no specifically the cost-based one. We might create lets say CPU-based autoscaler later with the requirement to scale-down during the rollover.

Copy link
Contributor

@kfaraz kfaraz Dec 22, 2025

Choose a reason for hiding this comment

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

because we're testing abstract autoscaler actions during the task rollover, no specifically the cost-based one.

I am not sure if this is entirely true, since cost-based auto-scaler is the only one that supports task count change on rollover right now.

The new test should be in the existing CostBasedAutoScalerIntegrationTest since it uses the same cluster setup (AFAICT) and verifies an important aspect of the cost-based auto-scaler.

In the future, when we add more auto-scalers, we can add separate tests for them.

kfaraz
kfaraz reviewed Dec 22, 2025
View reviewed changes
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up, @Fly-Style !
Left some initial drive-by comments. Will do a more thorough review later today/tomorrow.

@Fly-Style Fly-Style requested a review from kfaraz December 22, 2025 10:32
@Fly-Style Fly-Style marked this pull request as ready for review December 22, 2025 10:46
kfaraz
kfaraz reviewed Dec 23, 2025
View reviewed changes
@Fly-Style Fly-Style requested a review from kfaraz December 23, 2025 12:57
@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from 01f5fee to 2719a17 Compare December 23, 2025 13:07
@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from 2719a17 to 5936fd0 Compare December 23, 2025 13:08
@Fly-Style
Revert "Formattin"
640287c 
This reverts commit 5936fd0.
@Fly-Style
Revert "Formatting"
2722e0d 
This reverts commit 7788e38.
@Fly-Style
Format
2830ea1 
Signed-off-by: Sasha Syrotenko <alexander.syrotenko@imply.io>
@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from 6f3ce38 to 971f978 Compare December 23, 2025 13:17
@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from 971f978 to fd654e2 Compare December 23, 2025 13:18
@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from 065c5fa to 833cb88 Compare December 23, 2025 20:57
kfaraz
kfaraz reviewed Dec 24, 2025
View reviewed changes
.../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Outdated

if (taskAutoScaler != null && activelyReadingTaskGroups.isEmpty()) {
int rolloverTaskCount = taskAutoScaler.computeTaskCountForRollover();
if (rolloverTaskCount > 0 && rolloverTaskCount < ioConfig.getTaskCount()) {
Copy link
Contributor

@kfaraz kfaraz Dec 23, 2025

Choose a reason for hiding this comment

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

Should we also allow scale up on task rollover?

kfaraz
kfaraz approved these changes Jan 1, 2026
View reviewed changes
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

LGTM. +1 after CI passes.

@Fly-Style Fly-Style force-pushed the cost-autoscaler-task-rollout branch from a4b199f to 299ee81 Compare January 5, 2026 10:54
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 5, 2026
View reviewed changes
.../indexing/seekablestream/supervisor/SeekableStreamSupervisorScaleDuringTaskRolloverTest.java
@Override
protected OrderedSequenceNumber<String> makeSequenceNumber(String seq, boolean isExclusive)
{
return new OrderedSequenceNumber<>(seq, isExclusive)

Check warning

Code scanning / CodeQL

Inconsistent compareTo Warning test

This class declares
compareTo
Loading
but inherits equals; the two could be inconsistent.
...src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSupervisorSpecTest.java
EasyMock.expect(spec.isSuspended()).andReturn(true).anyTimes();
EasyMock.replay(spec);

EasyMock.expect(ingestionSchema.getIOConfig()).andReturn(seekableStreamSupervisorIOConfig).anyTimes();

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
SeekableStreamSupervisorSpec.getDataSchema
Loading
should be avoided because it has been deprecated.
...src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSupervisorSpecTest.java
EasyMock.replay(spec);

EasyMock.expect(ingestionSchema.getIOConfig()).andReturn(seekableStreamSupervisorIOConfig).anyTimes();
EasyMock.expect(ingestionSchema.getDataSchema()).andReturn(dataSchema).anyTimes();

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
SeekableStreamSupervisorSpec.getDataSchema
Loading
should be avoided because it has been deprecated.
.../indexing/seekablestream/supervisor/SeekableStreamSupervisorScaleDuringTaskRolloverTest.java
{
EasyMock.expect(spec.getId()).andReturn(SUPERVISOR).anyTimes();
EasyMock.expect(spec.getSupervisorStateManagerConfig()).andReturn(supervisorConfig).anyTimes();
EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes();

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
SeekableStreamSupervisorSpec.getDataSchema
Loading
should be avoided because it has been deprecated.
kfaraz
kfaraz reviewed Jan 5, 2026
View reviewed changes
.../indexing/seekablestream/supervisor/SeekableStreamSupervisorScaleDuringTaskRolloverTest.java
}

@Test
public void test_maybeScaleDuringTaskRollover_noAutoScaler_doesNotScale()
Copy link
Contributor

@kfaraz kfaraz Jan 5, 2026

Choose a reason for hiding this comment

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

Can we keep these test methods in the SupervisorSpecTest itself? Seems like we have copied over a lot of the boiler plate code that could have been avoided by keeping these methods there.

Alternatively, have these two tests extend some common base test class or use common test fixtures.

Copy link
Contributor Author

@Fly-Style Fly-Style Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Not really, I don't want to break incapsulation of maybeScaleDuringTaskRollover method for that purpose.
That was a point to introduce such test class - to be present under same package name.

Copy link
Contributor

@kfaraz kfaraz Jan 5, 2026

Choose a reason for hiding this comment

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

Oh, that should be easy to fix. The old SeekableStreamSupervisorSpecTest is itself in the wrong pacakge. 😂

Please move the existing test class to org.apache.druid.indexing.seekablestream.supervisor where both SeekableStreamSupervisor and SeekableStreamSupervisorSpec already live. Would this address your issue?

@kfaraz kfaraz merged commit ff6fc38 into apache:master Jan 5, 2026
75 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfaraz kfaraz kfaraz approved these changes

Assignees

No one assigned

Labels

Area - Ingestion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fly-Style @kfaraz