Conversation
|
Adding @brettsam for general WebJobs SDK review, as well as specific review for the DI and host builder changes, since he has a lot of experience in that area |
fabiocav
left a comment
fabiocav
left a comment
There was a problem hiding this comment.
Quick review on some of the DI changes. Sharing this, but will continue to iterate.
| services.TryAddSingleton<ITargetScalerManager, TargetScalerManager>(); | ||
|
|
||
| services.AddCommonScaleServices(); | ||
| services.AddSingleton<IScaleMetricsRepository, NullScaleMetricsRepository>(); |
There was a problem hiding this comment.
Consider using TryAddSingleton here instead.
| } | ||
|
|
||
| // Options logging | ||
| services.AddTransient(typeof(OptionsFactory<>)); |
There was a problem hiding this comment.
Why are we adding these options services as part of the scale logic? In the context of Functions, this is already handled, but for WebJobs, this shouldn't be tied to this logic.
There was a problem hiding this comment.
Yeah, it's not really. AddWebJobsScale configures a host as a ScaleHost, and we want the options logging stuff as well to log ScaleOptions. So it's common registrations that we want to do for normal WebJobs hosts as well as scaler hosts. Could be factored into a shared method like I did for AddCommonScaleServices.
There was a problem hiding this comment.
can you call services.AddOptions() instead? That conditionally registers everything you'd need for Options -- and if they change something in future versions, it'd continue to work.
There was a problem hiding this comment.
This was copy pasted from here. Do we want to change this in the both places: services.AddTransient(typeof(OptionsFactory<>)); -> services.AddOptions() ?
There was a problem hiding this comment.
Bunch of the tests are broken after the change:
https://github.com/Azure/azure-webjobs-sdk/pull/2954/checks?check_run_id=11964665844
Do we want to fix them all revert services.AddOptions()?
| /// <summary> | ||
| /// Manages scale monitoring operations. | ||
| /// </summary> | ||
| public class ScaleManager |
There was a problem hiding this comment.
Does this type need to be public?
Should we define (and register) an interface for this?
There was a problem hiding this comment.
We had defined an interface, but I just made it a public service type, similar to the decision I made for ConcurrencyManager. The rational is that it makes it easier to add new operations in the future w/o breaking an interface.
If we went the interface route, we wouldn't have an IScaleManager interface because that implies a larger capability set than just GetStatus. We'd end up defining just an IHostScaleStatusProvider or IScaleStatusProvider or some such interface for just that operation. I considered this.
| services.AddSingleton<IConcurrencyStatusRepository, BlobStorageConcurrencyStatusRepository>(); | ||
| } | ||
|
|
||
| public static void AddAzureStorageScaleServices(this IServiceCollection services) |
There was a problem hiding this comment.
Is this meant to ever be called by a customer?
There was a problem hiding this comment.
Yes, this will be called from ScaleController, see example here:
https://github.com/Azure/azure-webjobs-sdk/blob/scaler-apis/test/Microsoft.Azure.WebJobs.Host.EndToEndTests/Scale/ScaleHostEndToEndTests.cs#L84
Function host is not supposed to call this.
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the specified <see cref="IHostBuilder"/> as a scale manager host. |
There was a problem hiding this comment.
Is that something a customer will do? Or do we need to mention this is only for internal infrastructure or something like that?
There was a problem hiding this comment.
yes, this will be called from `ScaleController and a webjobs customer is not suppose to create a "scale manager host". I I extended the description.
| } | ||
|
|
||
| // Options logging | ||
| services.AddTransient(typeof(OptionsFactory<>)); |
There was a problem hiding this comment.
can you call services.AddOptions() instead? That conditionally registers everything you'd need for Options -- and if they change something in future versions, it'd continue to work.
| namespace Microsoft.Azure.WebJobs.Host.Scale | ||
| { | ||
| /// <summary> | ||
| /// Interface for providing a scale status |
There was a problem hiding this comment.
"Interface for providing aggregate scale status across all functions being monitored by the host."
| <Version>3.0.36$(VersionSuffix)</Version> | ||
| <HostStorageVersion>5.0.0-beta.2$(VersionSuffix)</HostStorageVersion> | ||
| <Version>3.0.37$(VersionSuffix)</Version> | ||
| <HostStorageVersion>5.0.0-beta.3$(VersionSuffix)</HostStorageVersion> |
There was a problem hiding this comment.
This package is about to be published as a GA (non-beta) release, OK to add the changes here?
There was a problem hiding this comment.
Updated to <HostStorageVersion>5.0.0$(VersionSuffix)</HostStorageVersion>
|
@alrod - Please update the PR description and would be great if you can add PRs links to extension SDK updates and Functions Host |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
6 participants
Resolves #2938
FunctionHost PR: Azure/azure-functions-host#9131
SB: Azure/azure-sdk-for-net#34557
EH: Azure/azure-sdk-for-net#35344
CosmosDb: Azure/azure-webjobs-sdk-extensions#841
Queue/Blob:Azure/azure-sdk-for-net#35358