Skip to content

Moving scale code to web jobs sdk to resuse in SC v3.#8977

Closed
alrod wants to merge 3 commits intodevfrom
alrod/sc-redesign
Closed

Moving scale code to web jobs sdk to resuse in SC v3.#8977
alrod wants to merge 3 commits intodevfrom
alrod/sc-redesign

Conversation

@alrod
Copy link
Copy Markdown
Member

@alrod alrod commented Dec 13, 2022 •
edited
Loading

Issue describing the changes in this PR

resolves Azure/azure-webjobs-sdk#2938

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@alrod alrod requested a review from a team as a code owner December 13, 2022 17:57
This was referenced Dec 13, 2022
Closed
Closed
liliankasem
liliankasem previously approved these changes Jan 18, 2023
View reviewed changes
Comment thread src/WebJobs.Script/Config/ScaleOptionsSetup.cs
public ScaleOptionsSetup(IEnvironment environment, IOptions<FunctionsHostingConfigOptions> functionsHostingConfigOptions)
{
_environment = environment;
_functionsHostingConfigOptions = functionsHostingConfigOptions;
Copy link
Copy Markdown
Member

@liliankasem liliankasem Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to throw if null?

_environment = environment ?? throw new ArgumentNullException(nameof(environment));
_functionsHostingConfigOptions = functionsHostingConfigOptions ?? ?? throw new ArgumentNullException(nameof(functionsHostingConfigOptions));

Comment thread src/WebJobs.Script/Config/ScaleOptionsSetup.cs

private bool IsTargetBasedScalingEnabledForTrigger(ITargetScaler targetScaler)
{
string assemblyName = targetScaler.GetType().Assembly.GetName().Name;
Copy link
Copy Markdown
Member

@liliankasem liliankasem Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
string assemblyName = targetScaler.GetType().Assembly.GetName().Name;
string assemblyName = targetScaler?.GetType().Assembly.GetName().Name;

Comment thread src/WebJobs.Script/Config/ScaleOptionsSetup.cs
{
string assemblyName = targetScaler.GetType().Assembly.GetName().Name;
string flag = _functionsHostingConfigOptions.Value.GetFeature(assemblyName);
return flag == "1";
Copy link
Copy Markdown
Member

@liliankasem liliankasem Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make "1" a constant with a name that describes what the 1 represents

madelinegordon
madelinegordon reviewed Jan 19, 2023
View reviewed changes
Comment thread test/WebJobs.Script.Tests/Scale/FunctionsScaleMonitorServiceTests.cs
var logs = _loggerProvider.GetAllLogMessages().ToArray();
Assert.Equal(1, logs.Length);
Assert.Equal("Runtime scale monitoring is enabled.", logs[0].FormattedMessage);
Assert.Equal("Scale monitor service started is started.", logs[0].FormattedMessage);
Copy link
Copy Markdown
Contributor

@madelinegordon madelinegordon Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - should the message here be "Scale monitor service is started" ?

@ghost ghost added the no-recent-activity label Jan 31, 2023
@ghost
Copy link
Copy Markdown

ghost commented Jan 31, 2023

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@ghost ghost added the no-recent-activity label Feb 7, 2023
@ghost
Copy link
Copy Markdown

ghost commented Feb 7, 2023

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@ghost ghost added the no-recent-activity label Feb 14, 2023
@ghost
Copy link
Copy Markdown

ghost commented Feb 14, 2023

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@liliankasem liliankasem self-requested a review February 23, 2023 23:54
@liliankasem
Copy link
Copy Markdown
Member

liliankasem commented Feb 24, 2023 •
edited
Loading

@alrod is this still blocked? WebJobs has been fully released

@mathewc
Copy link
Copy Markdown
Member

mathewc commented Feb 24, 2023

@alrod is this still blocked? WebJobs has been fully released

@liliankasem Yes this is blocked, and this PR probably should have been marked Draft since it relies on Azure/azure-webjobs-sdk#2954.

@liliankasem
Copy link
Copy Markdown
Member

liliankasem commented Feb 24, 2023

@alrod is this still blocked? WebJobs has been fully released

@liliankasem Yes this is blocked, and this PR probably should have been marked Draft since it relies on Azure/azure-webjobs-sdk#2954.

Thanks for the info! I'm going to go ahead and close this PR since it might be a while till 3.0.37 is out. Please open a new one when this is ready to go or reopen as a draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liliankasem liliankasem Awaiting requested review from liliankasem

2 more reviewers

@madelinegordon madelinegordon madelinegordon left review comments

@surgupta-msft surgupta-msft surgupta-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exposing common scaling code to consume by SC v3

5 participants

@alrod @liliankasem @mathewc @surgupta-msft @madelinegordon