Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
[data] introduce abstract interface for data autoscaling #45002
[data] introduce abstract interface for data autoscaling #45002
Changes from all commits
e8592ce
3f8bfa9
50d7217
b9e0297
45f69bc
18fb475
6b0fc9d
2253c82
c1dcd65
0af38bc
edb1725
74f9e13
3bd0f54
a853c52
11f96e0
c678dee
a225fc6
25f47a4
71d9922
856d806
9dcd9bc
406e40f
1b6bf5f
51ba4e9
75bdd26
b6fd417
71cacc4
774464a
9c47b7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this evaluate to true? Looks like
scale_up
always returns input value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i understand correctly, looks like scale_up is intended to return the number of actors actually added, which could differ from the requested scaleup. but in the current default implementation, looks like we return the input as @bveeramani said
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the current implementation always returns true. But I wanted to the make the interface more flexible and also make it consistent with
scale_down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect any operators other than
ActorPoolMapOperator
to override this method? Feel like it's not ideal to add a method to thePhysicalOperator
interface just for one subclass, although I can't think of any alternatives off the top of my head.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could potentially use some top PhysicalOperator level attribute which describes if the operator relies on actors or tasks, then call this method only for operators for which that attribute is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
streaming aggregation also use actor pools and may implement this. also, I want to avoid other components (autoscaler and resource manager) depending on the ActorPoolMapOperator. This makes the dependency graph more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is redundant, because we can tell this by if get_autoscaling_actor_pools returns an empty list.