Adds Idle feature to `SeekableStreamSupervisor` for inactive stream by tejaswini-imply · Pull Request #13144 · apache/druid

tejaswini-imply · 2022-09-26T10:26:51Z

Description

When a Kafka stream becomes inactive, i.e., no new data arrives, and all existing data is read, there is no need for Supervisor to create new indexing tasks.

Supervisor fetches current offsets every tuningConfig.offsetFetchPeriod (default - 30 sec) and compares them with latest offsets (fetched every ioConfig.period (default - 30 sec)) for lags. If the stream is inactive for dataSchema.ioConfig.idleConfig.inactiveAfterMillis, then supervisor state is turned to IDLE. (See conf section)
When Supervisor is in idle state it just doesn't create any new indexing tasks.
This temporary IDLE state is cleared when supervisor is restarted e.g. updated spec.
While the supervisor is in this temporary idle state, Supervisor will continue to check for any changes in the latest offsets (or if lag is > 0). If activity in the stream is detected again, then supervisor will come out of its IDLE state into RUNNING and resume creating indexing tasks.

Please note the initial delay before new data is ingested from when new data enters into the stream is the frequency of offsets fetch (30 sec) + initial warmup time to create new indexing tasks.

How current offsets fetching work:

Current offsets are fetched from running, publishing tasks for the partitions available and for the rest of the partitions from the metadata storage.

Configuration changes

dataSchema.ioConfig.idleConfig
- enabled - If true Supervisor goes into idle state when stream is inactive. The default value is false.
- inactiveAfterMillis - Minimum time interval to wait until stream is considered inactive/idle. The default value is 600,000 millis.
Adds new State - IDLE - to Basic states of Supervisor.

Key changed/added classes in this PR

SeekableStreamSupervisor
KafkaSupervisor
KafkaSupervisorIOConfig
SupervisorStateManager
IdleConfig

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.

lgtm-com · 2022-09-26T11:49:12Z

This pull request introduces 1 alert when merging d16f8d1 into 0bfa81b - view on LGTM.com

new alerts:

1 for Boxed variable is never null

lgtm-com · 2022-09-26T14:45:05Z

This pull request introduces 1 alert when merging 440544e into 0bfa81b - view on LGTM.com

new alerts:

1 for Boxed variable is never null

… changes.

… adds tests.

AmatyaAvadhanula · 2022-10-08T07:48:21Z

@tejaswini-imply thank you for the changes and tests.
However, ITKafkaIndexingServiceNonTransactionalParallelizedTest.testIndexDataWithIdleBehaviourEnabled seems to be failing.
Could you please set the environment to enable cluster-wide idle behaviour?

AmatyaAvadhanula

@tejaswini-imply Thank you for the changes. LGTM

kfaraz

Thanks for the feature, @tejaswini-imply !
I have added a bunch of comments, mostly minor style suggestions.

Around the nomenclature of the new configs, naming a config behaviour or feature seems a little redundant.
I would prefer something like:
druid.supervisor.idleWhenInactive
or druid.server.setIdleWhenInactive
and
druid.supervisor.idleWaitTimeMillis.

kfaraz · 2022-10-10T11:23:52Z

    }

-    // if we're trying to switch to a healthy steady state (i.e. RUNNING or SUSPENDED) but haven't had a successful run
+    // if we're trying to switch to a healthy steady state (i.e. RUNNING or SUSPENDED) or IDLE state but haven't had a successful run


I don't think this change is applicable since IDLE doesn't count as a "healthy steady state". IIUC, the healthySteadyState can only be RUNNING or SUSPENDED, which is determined while creating this SupervisorStateManager instance.

IDLE isn't a healthy steady state. Hence it's mentioned healthy steady state or IDLE state. Please let me know if the way it's mentioned is confusing.

kfaraz · 2022-10-10T11:32:46Z

@@ -0,0 +1,60 @@
+{


This spec seems pretty generic. Do we really need to add a new one for the new test or can we reuse any existing one?

kfaraz

Added minor comments, otherwise looks good.

kfaraz · 2022-10-12T06:34:06Z

+         "idleConfig": {
+           "enabled": true,
+           "inactiveAfterMillis": 600000 
+         }


nit: missing comma

kfaraz · 2022-10-12T06:36:03Z

+
+import javax.annotation.Nullable;
+
+public class IdleConfig


Please add a javadoc.

abhishekagarwal87 · 2022-10-12T13:01:32Z

Thank you @tejaswini-imply. This is going to be very useful feature.

tejaswini-imply · 2022-10-12T14:08:22Z

Thanks @AmatyaAvadhanula, @kfaraz and @abhishekagarwal87 for thorough review.

Fixes inclusion of all stream partitions in all tasks. The PR (Adds Idle feature to `SeekableStreamSupervisor` for inactive stream) - #13144 updates the resulting lag calculation map in `KafkaSupervisor` to include all the latest partitions from the stream to set the idle state accordingly rather than the previous way of lag calculation only for the partitions actively being read from the stream. This led to an explosion of metrics in lag reports in cases where 1000s of tasks per supervisor are present. Changes: - Add a new method to generate lags for only those partitions a single task is actively reading from while updating the Supervisor reports.

…pache#13144) * Idle Seekable stream supervisor changes. * nit * nit * nit * Adds unit tests * Supervisor decides it's idle state instead of AutoScaler * docs update * nit * nit * docs update * Adds Kafka unit test * Adds Kafka Integration test. * Updates travis config. * Updates kafka-indexing-service dependencies. * updates previous offsets snapshot & doc * Doesn't act if supervisor is suspended. * Fixes highest current offsets fetch bug, adds new Kafka UT tests, doc changes. * Reverts Kinesis Supervisor idle behaviour changes. * nit * nit * Corrects SeekableStreamSupervisorSpec check on idle behaviour config, adds tests. * Fixes getHighestCurrentOffsets to fetch offsets of publishing tasks too * Adds Kafka Supervisor UT * Improves test coverage in druid-server * Corrects IT override config * Doc updates and Syntactic changes * nit * supervisorSpec.ioConfig.idleConfig changes

Fixes inclusion of all stream partitions in all tasks. The PR (Adds Idle feature to `SeekableStreamSupervisor` for inactive stream) - apache#13144 updates the resulting lag calculation map in `KafkaSupervisor` to include all the latest partitions from the stream to set the idle state accordingly rather than the previous way of lag calculation only for the partitions actively being read from the stream. This led to an explosion of metrics in lag reports in cases where 1000s of tasks per supervisor are present. Changes: - Add a new method to generate lags for only those partitions a single task is actively reading from while updating the Supervisor reports.

tejaswini-imply added 2 commits September 26, 2022 13:09

Idle Seekable stream supervisor changes.

05b5b32

Merge branch 'master' into feature-idle_supervisor

d16f8d1

tejaswini-imply added 2 commits September 26, 2022 18:47

nit

48c6ba7

nit

440544e

kfaraz added the Area - Streaming Ingestion label Sep 27, 2022

nit

95208c8

abhishekagarwal87 added Design Review Release Notes labels Sep 27, 2022

Adds unit tests

d489290

AmatyaAvadhanula reviewed Sep 28, 2022

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Outdated

tejaswini-imply added 3 commits September 30, 2022 01:14

Supervisor decides it's idle state instead of AutoScaler

090090b

docs update

d5238cb

nit

eba264d

AmatyaAvadhanula reviewed Sep 30, 2022

View reviewed changes

Comment thread docs/configuration/index.md Outdated

tejaswini-imply added 2 commits September 30, 2022 13:36

nit

a12245e

docs update

764ffcd

tejaswini-imply marked this pull request as ready for review September 30, 2022 09:43

Adds Kafka unit test

0a22194

AmatyaAvadhanula reviewed Oct 5, 2022

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java

AmatyaAvadhanula reviewed Oct 5, 2022

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Outdated

tejaswini-imply added 6 commits October 6, 2022 12:57

Adds Kafka Integration test.

3d8327e

Updates travis config.

cdd4369

Merge branch 'master' into feature-idle_supervisor

81b030c

Updates kafka-indexing-service dependencies.

98a05ea

updates previous offsets snapshot & doc

c0df3d7

Doesn't act if supervisor is suspended.

edacbfd

AmatyaAvadhanula reviewed Oct 7, 2022

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Outdated

AmatyaAvadhanula reviewed Oct 7, 2022

View reviewed changes

Comment thread .../main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java

tejaswini-imply added 8 commits October 7, 2022 15:06

Fixes highest current offsets fetch bug, adds new Kafka UT tests, doc…

f084da9

… changes.

Reverts Kinesis Supervisor idle behaviour changes.

b81c656

nit

c94bd68

nit

9a02c66

Corrects SeekableStreamSupervisorSpec check on idle behaviour config,…

6b463e4

… adds tests.

Fixes getHighestCurrentOffsets to fetch offsets of publishing tasks too

b0d5286

Adds Kafka Supervisor UT

9cc358c

Improves test coverage in druid-server

f85d104

AmatyaAvadhanula reviewed Oct 8, 2022

View reviewed changes

Comment thread ...n/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java Outdated

tejaswini-imply added 2 commits October 8, 2022 14:44

Corrects IT override config

958549a

Merge branch 'master' into feature-idle_supervisor

34980c0

AmatyaAvadhanula approved these changes Oct 10, 2022

View reviewed changes

kfaraz requested changes Oct 10, 2022

View reviewed changes

tejaswini-imply added 3 commits October 10, 2022 21:49

Doc updates and Syntactic changes

bc5eab8

nit

5e73939

supervisorSpec.ioConfig.idleConfig changes

4b5bc3f

kfaraz approved these changes Oct 12, 2022

View reviewed changes

abhishekagarwal87 merged commit 3e13584 into apache:master Oct 12, 2022

tejaswini-imply mentioned this pull request Oct 17, 2022

Flaky IT: ITKafkaIndexingServiceNonTransactionalParallelizedTest #13203

Closed

tejaswini-imply mentioned this pull request Nov 17, 2022

Fixes Kafka Supervisor Lag Report #13380

Merged

5 of 10 tasks

kfaraz added this to the 25.0 milestone Nov 22, 2022

This was referenced Dec 18, 2022

[Draft] 25.0.0 Release Notes #13592

Closed

Add SegmentAllocationQueue to batch allocation actions #13369

Merged

wiegandf mentioned this pull request Mar 15, 2023

Persist idle status of supervisors #13936

Closed

Conversation

tejaswini-imply commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How current offsets fetching work:

Configuration changes

Key changed/added classes in this PR

Uh oh!

lgtm-com Bot commented Sep 26, 2022

Uh oh!

lgtm-com Bot commented Sep 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AmatyaAvadhanula commented Oct 8, 2022

Uh oh!

Uh oh!

AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

tejaswini-imply Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfaraz Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

kfaraz Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 commented Oct 12, 2022

Uh oh!

tejaswini-imply commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tejaswini-imply commented Sep 26, 2022 •

edited

Loading