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.

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

bachuv
Copy link
Collaborator

@bachuv bachuv commented May 3, 2023

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:

  • Add NetheriteMetricsProvider and move CollectMetrics code from ScaleMonitor here
  • Add NetheriteTargetScaler (implements ITargetScaler)
  • Override TryGetTargetScaler in NetheriteProvider
  • Add unit tests
  • E2E testing

@bachuv bachuv self-assigned this May 3, 2023
@bachuv bachuv changed the title [WIP] Add target based scaling support [WIP] Add target based scaling support for Netherite May 4, 2023
@sebastianburckhardt
Copy link
Member

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.

@bachuv
Copy link
Collaborator Author

bachuv commented May 9, 2023

Thanks Sebastian! I updated the PR and tested your changes locally.

@bachuv bachuv marked this pull request as ready for review May 9, 2023 16:38
@bachuv bachuv changed the title [WIP] Add target based scaling support for Netherite Add target based scaling support for Netherite May 9, 2023
@bachuv
Copy link
Collaborator Author

bachuv commented May 9, 2023

@sebastianburckhardt, can you point me to where I should add unit tests?

@sebastianburckhardt
Copy link
Member

@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.

Comment on lines +40 to +41
[InlineData(10, 10, 2)]
[InlineData(20, 20, 2)]
Copy link
Collaborator Author

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.

Comment on lines +47 to +51
var loadInformation = new Dictionary<uint, PartitionLoadInfo>()
{
{ 1, this.Create("A") },
{ 2, this.Create("B") },
};
Copy link
Collaborator Author

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?

@sebastianburckhardt
Copy link
Member

Is this PR still needed?

@davidmrdavid
Copy link
Member

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?

@bachuv
Copy link
Collaborator Author

bachuv commented Mar 15, 2024

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.

@davidmrdavid, yes that's right.

@davidmrdavid davidmrdavid marked this pull request as draft March 15, 2024 23:46
@davidmrdavid
Copy link
Member

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!

@ericleigh007
Copy link

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.

@davidmrdavid
Copy link
Member

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)

@ericleigh007
Copy link

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 does seem to look different than the result we get with AzureStorage.
As another peripheral point, I thought that target-based scaling pivoted back to metric-based because mainly lack of management roles, but that may be an incorrect perception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants