Skip to content
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

Working on settings / config sources. #4164

Merged
merged 14 commits into from
Nov 9, 2019
Merged

Working on settings / config sources. #4164

merged 14 commits into from
Nov 9, 2019

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Sep 4, 2019

This is on the continuation of #4001 defining caching rules, also related to #4107.

  • One rule being to update internal data by re-reading a store (shared or not), not just relying on the fact that we have updated them just before storing them. There are others places that would need to be updated, here i applied this rule to the shell settings / configurations.

  • So, here when we update a tenant and reload it, the related shell settings are reloaded so that after having updated a given tenant through tenants.json or its specific appsettings.json, if i reload it through the admin i can see the changes under the tenants page (was not the case before).

    I think it's a more complete tenant reloading. Then in a distributed environment where a distributed event will be sent on a tenant reloading, on each instance this tenant will be reloaded by loading and using the updated settings coming e.g from a shared source.

    If an library API allows to update a shared source from an OC app e.g through the admin, the distributed event could be sent automatically. We could then think about a config provider based on Redis, maybe after on blob, database ...

  • Note: Maybe an idea to also have in the admin a soft app restart that would reload all tenants.

Note: For a config provider based on the database we could use a light scope for a given tenant as we do to see if a given shell descriptor exists. Or a IDbConnectionAccessor as used in ShellDescriptorManager by UpgradeFromBeta2. In both case, on saving a new transaction would need to be created and committed.

Question: Can we remove UpgradeFromBeta2(), hmm at least in the 3.0 branch, notice that this code is always executed when creating a new tenant.

@jtkech
Copy link
Member Author

jtkech commented Sep 8, 2019

  • UpgradeFromBeta2 has been removed in the 3.0 branch.

  • I could make ShellConfigurationSources.Save and ShellsSettingsSources.Save async (i lost my changes but easy to redo). Maybe useful if an implementation is able to update an external source, let me know.

@jtkech
Copy link
Member Author

jtkech commented Sep 16, 2019

As i have time i will re-think about making save settings async.
If it could be useful when storing data in a shared external store.

@@ -339,7 +346,7 @@ public Task ReloadShellContextAsync(ShellSettings settings)
context.Release();
}

return GetOrCreateShellContextAsync(settings);
return GetOrCreateShellContextAsync(settings, true);
Copy link
Member

@sebastienros sebastienros Oct 15, 2019

Choose a reason for hiding this comment

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

Load the settings before:

if (settings.State != TenantState.Initializing)
{
  settings = _shellSettingsManager.LoadSettings(settings.Name);
}

Remove the boolean to reload settings.

@jtkech
Copy link
Member Author

jtkech commented Oct 31, 2019

@sebastienros i think this one is ready, we already reviewed it together ;)

Update: Fixes #4734

@jtkech
Copy link
Member Author

jtkech commented Nov 9, 2019

@sebastienros

So, after merging dev, i began to update the files related to #2450 Tenant Workflow, but right now i just fixed the build on my side because seems that there is more work to do

  • The enable and disable tasks does nothing.

  • For other tasks, i would say that in place of using new ShellSettings() in some places, we would have to use

    // Creates a default shell settings based on the configuration.
    shellSettings = ShellSettingsManager.CreateDefaultSettings();
    

    But just saw that the default ShellSettings ctor now does the same, so that's ok.

    But after we would need to take into account preconfigured data in place of overriding all of them in the create / setup tenant tasks.

  • The create task saves the settings and returns Done even if the tenant name is not provided. Maybe intended so that a next setup task will provide it, but see below.

  • In the setup task, if the settings related to the provided tenant name is not found, we create a new settings but not with all data. I assume that the setup task is intended to execute after the create tenant task to provide remaining data to do a setup. So, if we don't retrieve the previous settings related to a tenant name, maybe better to just return Failed in this case.

  • It doesn't follow the save / reload pattern, e.g LoadSettings() in place of TryGetSettings(), this to take into account all the configuration stack. But this is related to what my PR has to do.

Maybe i misunderstood some things, so i would need to tried it, well understand it and then update it.

The problem with my PR, idem for #4001, is that, if it takes a too long time to be merged, it becomes not so easy to maintain, e.g to fix those that are related but merged before.

So not so motivated, i would be if this PR and #4001 will not take again a long time to be merged.

@jtkech jtkech merged commit 061ba64 into dev Nov 9, 2019
@jtkech jtkech deleted the jtkech/settings branch November 14, 2019 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants