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

Make #16284 backward compatible #16334

Merged
merged 6 commits into from
Apr 29, 2024
Merged

Make #16284 backward compatible #16334

merged 6 commits into from
Apr 29, 2024

Conversation

adithyachakilam
Copy link
Contributor

In #16284, we changed the way how kinesis autoscaler computes the lag for the purposes of autoscaling. Put that feature behind a flag so as to not annoy customers who have already configured the auto scaler thesholds based on the total lag.

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-advanced-security[bot]
github-advanced-security bot found potential problems Apr 26, 2024
View reviewed changes
.../java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScaler.java
long lag = lagBasedAutoScalerConfig.getLagStatsType() != null ?
lagStats.getMetric(lagBasedAutoScalerConfig.getLagStatsType()) :
lagStats.getPrefferedScalingMetric();
lagMetricsQueue.offer(lag > 0 ? lag : 0L);

Check notice

Code scanning / CodeQL

Ignored error status of call Note

Method run ignores exceptional return value of CircularFifoQueue.offer.
.../java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScaler.java
lagStats.getPrefferedScalingMetric();
lagMetricsQueue.offer(lag > 0 ? lag : 0L);
} else {
lagMetricsQueue.offer(0L);

Check notice

Code scanning / CodeQL

Ignored error status of call Note

Method run ignores exceptional return value of CircularFifoQueue.offer.
kfaraz
kfaraz reviewed Apr 27, 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.

@adithyachakilam , thanks for the changes! The implementation makes sense. I have just left some suggestions for field names and code structure.

docs/ingestion/supervisor.md Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java Outdated Show resolved Hide resolved
.../java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScaler.java Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java Outdated Show resolved Hide resolved
...er/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/ScalingMetric.java Outdated Show resolved Hide resolved
...er/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/ScalingMetric.java Outdated Show resolved Hide resolved
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java Outdated Show resolved Hide resolved
...org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java Outdated Show resolved Hide resolved
kfaraz
kfaraz reviewed Apr 28, 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.

Final comments, will approve once these are addressed.

.../java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScaler.java Show resolved Hide resolved
...org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java Outdated Show resolved Hide resolved
...org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java Show resolved Hide resolved
...org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java Outdated Show resolved Hide resolved
...src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSupervisorSpecTest.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/AggregateFunction.java Outdated Show resolved Hide resolved
kfaraz
kfaraz approved these changes Apr 29, 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.

Thanks for addressing the comments, @adithyachakilam .

@kfaraz
Copy link
Contributor

kfaraz commented Apr 29, 2024

Merging this PR as the pending tests are stuck due to the issue described in #16347 . The same tests have already passed on a different JDK.

@kfaraz kfaraz merged commit f8015eb into apache:master Apr 29, 2024
85 of 87 checks passed
@suneet-s
Copy link
Contributor

@adithyachakilam Can you please backport this change to the 30.0 branch as it looks like #16284 is in that branch

adithyachakilam added a commit to adithyachakilam/druid that referenced this pull request May 2, 2024
@adithyachakilam
Add config lagAggregate to LagBasedAutoScalerConfig (apache#16334)
4563812 
Changes:
- Add new config `lagAggregate` to `LagBasedAutoScalerConfig`
- Add field `aggregateForScaling` to `LagStats`
- Use the new field/config to determine which aggregate to use to compute lag
- Remove method `Supervisor.computeLagForAutoScaler()`
@adithyachakilam adithyachakilam mentioned this pull request May 2, 2024
Merged
kfaraz pushed a commit that referenced this pull request May 3, 2024
@adithyachakilam
Add config lagAggregate to LagBasedAutoScalerConfig (#16334) (#16377)
d685877 
Changes:
- Add new config `lagAggregate` to `LagBasedAutoScalerConfig`
- Add field `aggregateForScaling` to `LagStats`
- Use the new field/config to determine which aggregate to use to compute lag
- Remove method `Supervisor.computeLagForAutoScaler()`
@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

@suneet-s suneet-s suneet-s left review comments

@kfaraz kfaraz kfaraz approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@adithyachakilam @kfaraz @suneet-s