-
Notifications
You must be signed in to change notification settings - Fork 184
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
Fix MetadataProvider initialization in Hot Reload and remove duplicate Query Executor reloading #2428
base: main
Are you sure you want to change the base?
Fix MetadataProvider initialization in Hot Reload and remove duplicate Query Executor reloading #2428
Changes from 4 commits
ad0724c
b36a1a9
f6d2366
e257ad3
0286c2b
045b8cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,6 @@ public HotReloadEventHandler() | |
{ METADATA_PROVIDER_FACTORY_ON_CONFIG_CHANGED, null }, | ||
{ QUERY_ENGINE_FACTORY_ON_CONFIG_CHANGED,null }, | ||
{ MUTATION_ENGINE_FACTORY_ON_CONFIG_CHANGED,null }, | ||
{ 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 }, | ||
Comment on lines
-29
to
-32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add comment surrounding |
||
{ DOCUMENTOR_ON_CONFIG_CHANGED, null } | ||
}; | ||
} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add more details to the PR description for why we need to change how |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,10 +56,6 @@ protected void SendEventNotification(string message = "") | |
OnConfigChangedEvent(new HotReloadEventArgs(METADATA_PROVIDER_FACTORY_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(QUERY_ENGINE_FACTORY_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(MUTATION_ENGINE_FACTORY_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(MSSQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(MYSQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(POSTGRESQL_QUERY_EXECUTOR_ON_CONFIG_CHANGED, message)); | ||
OnConfigChangedEvent(new HotReloadEventArgs(DOCUMENTOR_ON_CONFIG_CHANGED, message)); | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Dictionary<string, string>? datasourceNameToConnectionString = null, | ||
EnvironmentVariableReplacementFailureMode replacementFailureMode = EnvironmentVariableReplacementFailureMode.Throw) | ||
{ | ||
|
@@ -113,13 +108,7 @@ public static bool TryParseConfig(string json, | |
} | ||
|
||
// retreive current connection string from config | ||
string updatedConnectionString = config.DataSource.ConnectionString; | ||
|
||
// set dataSourceName to default if not provided | ||
if (string.IsNullOrEmpty(dataSourceName)) | ||
{ | ||
dataSourceName = config.DefaultDataSourceName; | ||
} | ||
string updatedConnectionString = config.DataSource.ConnectionString; | ||
|
||
if (!string.IsNullOrEmpty(connectionString)) | ||
{ | ||
|
@@ -133,7 +122,7 @@ public static bool TryParseConfig(string json, | |
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should put |
||
|
||
// add to dictionary if datasourceName is present (will either be the default or the one provided) | ||
datasourceNameToConnectionString.TryAdd(dataSourceName, updatedConnectionString); | ||
datasourceNameToConnectionString.TryAdd(config.DefaultDataSourceName, updatedConnectionString); | ||
|
||
// iterate over dictionary and update runtime config with connection strings. | ||
foreach ((string dataSourceKey, string connectionValue) in datasourceNameToConnectionString) | ||
|
@@ -153,7 +142,7 @@ public static bool TryParseConfig(string json, | |
} | ||
|
||
ds = ds with { ConnectionString = updatedConnection }; | ||
config.UpdateDataSourceNameToDataSource(dataSourceName, ds); | ||
config.UpdateDataSourceNameToDataSource(config.DefaultDataSourceName, ds); | ||
|
||
if (string.Equals(dataSourceKey, config.DefaultDataSourceName, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just for new allocation, we shouldn't change its access modifier |
||
private readonly RuntimeConfigProvider _runtimeConfigProvider; | ||
private readonly IAbstractQueryManagerFactory _queryManagerFactory; | ||
private readonly ILogger<ISqlMetadataProvider> _logger; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
ConfigureMetadataProviders(); | ||
// Blocks the current thread until initialization is finished. | ||
this.InitializeAsync().GetAwaiter().GetResult(); | ||
aaronburtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/// <inheritdoc /> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add more detail in pr description for how you manually tested these changes. Use
#2417 and #2414 as examples via the "test scenarios" sections