SubscriptionProcessor is now set up to use ScopedMessageHandler #473
Conversation
…fault, so that individual jobs don't have to do same setup over and over.
agr
requested review from
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
chenriksson,
cristinamanum,
skofman1 and
scottbommarito
|
|
||
| internal override void ConfigureAutofacServicesInternal(ContainerBuilder containerBuilder) | ||
| { | ||
| const string validateCertificateBindingKey = "SubscriptionProcessorJob_SubscriptionProcessorKey"; |
There was a problem hiding this comment.
How about just bindingKey?
| protected abstract void ConfigureJobServices(IServiceCollection services, IConfigurationRoot configurationRoot); | ||
| internal virtual void ConfigureAutofacServicesInternal(ContainerBuilder containerBuilder) |
There was a problem hiding this comment.
No one except JsonConfigurationJob should call ConfigureAutofacServicesInternal, right? Instead of adding ConfigureAutofacServicesInternal, what do you think of calling base.ConfigureAutofacServices in all the jobs? The message handler could then be configured in that method.
There was a problem hiding this comment.
It is even easier to forget to call base.ConfigureAutofacServices than to forget to set up the scope message handler. The idea was that it would work without any effort on the job side, and effort us required to opt out of the behavior.
There was a problem hiding this comment.
It was rewritten so SubcriptionProcessorJob<T> now provides the ConfigureDefaultSubscriptionProcessor with default configuration and it is individual job's task to call it when needed. Checks are in place that suggest proper setup.
| @@ -184,7 +185,12 @@ private void ConfigureLibraries(IServiceCollection services) | |||
| services.AddLogging(); | |||
| } | |||
|
|
|||
| protected abstract void ConfigureAutofacServices(ContainerBuilder containerBuilder); | |||
| protected virtual void ConfigureAutofacServices(ContainerBuilder containerBuilder) | |||
There was a problem hiding this comment.
Could you add comments for the purpose of these two methods? Why is the internal one needed?
… not configured in the DI, suggesting to use the default setup from `SubcriptionProcessorJob<T>`. Update jobs code to include a call to that helper method.
|
Tested affected services in dev. |
[ReleasePrep][2019.01.14] RI dev to master. Take 2
* SubscriptionProcessor is now set up to use ScopedMessageHandler by default, so that individual jobs don't have to do same setup over and over. * Added specific check for the case when `ISubscriptionProcessor<T>` is not configured in the DI, suggesting to use the default setup from `SubcriptionProcessorJob<T>`. Update jobs code to include a call to that helper method.
[ReleasePrep][2019.01.14] RI dev to master. Take 2
Addresses https://github.com/NuGet/Engineering/issues/1501 and https://github.com/NuGet/Engineering/issues/1486
No more copying the same DI setup for injecting the
ScopedMessageHandlerintoSubscriptionProcessorin every job that derives fromSubcriptionProcessorJob<T>, easy to forget to do that setup and the consequences are weird.