-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add target based scaling support for Netherite #265
base: main
Are you sure you want to change the base?
Conversation
I have spent some time to modify the logic so it now computes a target instead of a scale recommendation. It still has the same overall structure but I had to use the data provided by the metrics to determine the current scale. |
Thanks Sebastian! I updated the PR and tested your changes locally. |
@sebastianburckhardt, can you point me to where I should add unit tests? |
I think the best place to add the tests is the DurableTask.Netherite.Tests project. The other tests in there are mostly end-to-end tests that use some special fixture. In your case, just plain normal unit tests are probably enough, so I don't think you need to use any of those. |
[InlineData(10, 10, 2)] | ||
[InlineData(20, 20, 2)] |
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.
@sebastianburckhardt, can you help provide expected input and output values to test the target based scaling algorithm? The inputs for this test are maxConcurrentTaskActivityWorkItems
and maxConcurrentTaskOrchestrationWorkItems
.
var loadInformation = new Dictionary<uint, PartitionLoadInfo>() | ||
{ | ||
{ 1, this.Create("A") }, | ||
{ 2, this.Create("B") }, | ||
}; |
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.
@sebastianburckhardt, I copied this from another test setup so let me know if I'm using it correctly. Should this information get sent as an input parameter with different values for each test case?
Is this PR still needed? |
I believe we've parked this for the time being. Right @bachuv? I suspect this may become relevant again once Flex Consumption enter it's GA-prep phase, but we're not there yet. @sebastianburckhardt: is it a problem if this remains open, just perhaps in draft mode? |
@davidmrdavid, yes that's right. |
I've taken the liberty of marking this as draft to help signal that it doesn't need active review, and is simply an idle PR meant for future completion. Hope that's ok! |
Not to be too obvious, but I guess this is why we were often peppered with log entries warning that target-based scaling could not be used. The log entries are quite noisy. |
Hey @ericleigh007 - do you know if your app somehow "opted in" to target based scaling? If you're getting these warnings without opting in, that could be a problem (and possibly a reason to re-prioritize this work) |
I was under the impression that target-based scaling was the default, and you had to opt-out. So we have done nothing specifically for this. |
This PR adds support for target based scaling. It builds upon Azure/azure-functions-durable-extension#2452. This PR is in draft mode since it hasn't been tested E2E yet.
Changes:
NetheriteMetricsProvider
and moveCollectMetrics
code from ScaleMonitor hereNetheriteTargetScaler
(implementsITargetScaler
)TryGetTargetScaler
in NetheriteProvider