Fix MetadataProvider initialization in Hot Reload and remove duplicate Query Executor reloading #2428
Conversation
aaronburtle
requested review from
Aniruddh25,
seantleonard,
abhishekkumams,
RubenCerna2079,
ravishetye,
rohkhann,
neeraj-sharma2592,
sourabh1007 and
sezal98
as code owners
aaronburtle
added
bug
src/Config/RuntimeConfigLoader.cs
Outdated
There was a problem hiding this comment.
can you add more details to the PR description for why we need to change how dataSourceName is used: using the passed in argument versus using the "default" data source name.
| { QUERY_EXECUTOR_ON_CONFIG_CHANGED, null }, | ||
| { MSSQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, null }, | ||
| { MYSQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, null }, | ||
| { POSTGRESQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, null }, |
There was a problem hiding this comment.
Can you add comment surrounding QUERY_MANAGER_FACTORY_ON_CONFIG_CHANGED here that describes how the "query executors since those are already being correctly refreshed already by the query manager factory reload process."
| @@ -17,7 +17,7 @@ namespace Azure.DataApiBuilder.Core.Services.MetadataProviders | |||
| /// <inheritdoc /> | |||
| public class MetadataProviderFactory : IMetadataProviderFactory | |||
| { | |||
| private readonly IDictionary<string, ISqlMetadataProvider> _metadataProviders; | |||
| private IDictionary<string, ISqlMetadataProvider> _metadataProviders; | |||
There was a problem hiding this comment.
just for new allocation, we shouldn't change its access modifier
| @@ -62,7 +62,10 @@ private void ConfigureMetadataProviders() | |||
|
|
|||
| public void OnConfigChanged(object? sender, HotReloadEventArgs args) | |||
| { | |||
| _metadataProviders = new Dictionary<string, ISqlMetadataProvider>(); | |||
There was a problem hiding this comment.
better to clear() because it avoids re-allocating the _metadataProviders dictionary every time the OnConfigChanged method is called. it is more efficient in terms of memory and performance.
I'll address it in my PR. so you can remove revert this
| @@ -97,7 +93,6 @@ public static bool TryParseConfig(string json, | |||
| ILogger? logger = null, | |||
| string? connectionString = null, | |||
| bool replaceEnvVar = false, | |||
| string dataSourceName = "", | |||
There was a problem hiding this comment.
string dataSourceName = "",
If this has been removed, we should modify the doc string
| @@ -133,7 +122,7 @@ public static bool TryParseConfig(string json, | |||
| } | |||
There was a problem hiding this comment.
We should put Dictionary<string, string>? datasourceNameToConnectionString = new Dictionary<string, string>(); in the function defination itself instead of putting null there and then checking and replacing it with new object here.
Why make this change?
Closes #2426
Closes #2427
What is this change?
Properly create and initialize a new set of
SqlMetadataProvidersas part of the hot-reload process of theSqlMetadataProviderFactory, by utilizing theinitiaizeAsync()function. This guarantees a correctly configured set of meta data providers for the factory.Remove the event handling to hot-reload the
QueryExecutorsclasses specifically, since this is already handled during the hot-reload process of theQueryManagerFactory. That process creates new and up to dateQueryExecutorsas part of a hot-reload.With the correct initialization of the meta data providers, we can remove the code that saves the
DefaultDataSourceNameto be passed along the hot-reloaded configs. With all of the data structures we initialized on new meta data providers, we do not need to save the data source name and hot-reloading will be more resilient.How was this tested?
Manually ran to verify hot reload works and requests work before and after hot reloads.
Sample Request(s)
Any rest request should work.
eg:
https://localhost:5001/rest/Book