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

Check if supervisor could be idle on startup #16844

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Check if supervisor could be idle on startup #16844

merged 9 commits into from
Aug 9, 2024

Conversation

adithyachakilam
Copy link
Contributor

Fixes #13936.

Description

In cases where a supervisor is idle and the overlord is restarted for some reason, the supervisor would start spinning tasks again. In clusters where there are many low throughput stream this spike the task count unnecessarily. This PR checks if the latest offset with that of the ones committed in metadata during the startup of supervisor and sets it idle state if they match.

Release note

Supervisor would not go back into running state from the idle if the overlord restart happens for some reason.

Key changed/added classes in this PR
  • SeekableStreamSupervisor

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.

@adithyachakilam adithyachakilam changed the title Check if supervisor is idle on startup Check if supervisor could be idle on startup Aug 6, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 6, 2024
View reviewed changes
...a/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java
).anyTimes();
EasyMock.reset(spec);
EasyMock.expect(spec.isSuspended()).andReturn(false).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 Aug 8, 2024
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.

Minor comments, rest looks good to me.

.../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Outdated Show resolved Hide resolved
.../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Show resolved Hide resolved
georgew5656
georgew5656 approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@georgew5656 georgew5656 left a comment

Choose a reason for hiding this comment

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

thought through all the possible cases and i think this will work. i think some comments in the logic would be helpful though, like explaining each scenario

  1. running supervisor with data, overlord restarts
  2. new supervisor with data on running overlord
  3. running supervisor that stops getting data

for future changes to the code

adithyachakilam
adithyachakilam commented Aug 8, 2024
View reviewed changes
...ng-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java
verifyAll();

int taskCountAfterScale = supervisor.getIoConfig().getTaskCount();
Assert.assertEquals(2, taskCountAfterScale);
Assert.assertEquals(SupervisorStateManager.BasicState.IDLE, supervisor.getState());
Copy link
Contributor Author

@adithyachakilam adithyachakilam Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is an invalid test. There are no actuals tasks running to consume the lag and go to idle. It used to work previously because reporting thread is yet to be executed before the completion of test and computeLag implied null as 0 lag. But with us now updating the lag proactively this fails.

@kfaraz kfaraz merged commit a7dd436 into apache:master Aug 9, 2024
90 checks passed
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
@kfaraz kfaraz mentioned this pull request Oct 11, 2024
Closed
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

@georgew5656 georgew5656 georgew5656 approved these changes

Assignees
No one assigned
Labels
Area - Ingestion Area - Streaming Ingestion
Projects
None yet
Milestone
31.0.0
Development

Successfully merging this pull request may close these issues.

Persist idle status of supervisors
3 participants
@adithyachakilam @georgew5656 @kfaraz