Remove secondary caching in ContentDefinitionManager, improve error message in DataMigrationManager #16212
Conversation
| if (_scopedTypeDefinitions.TryGetValue(name, out var typeDefinition)) | ||
| { | ||
| return typeDefinition; | ||
| } |
There was a problem hiding this comment.
@sebastienros should be the problem here, the Loadxx family of methods should no longer fetch content from the cache
/cc @MikeAlhayek
There was a problem hiding this comment.
Hi @MikeAlhayek , do I need to revert these changes? Or do I keep them?
Because there is already a cache in the ContentDefinitionStore and a second-level cache would make the logic more complex
There was a problem hiding this comment.
If we use both MemoryCache and Redis caching, it may have problems enabling distributed caching and multi-server deployments
hyzx86
changed the title
|
Found some other issues, should I just change it here? Or are there other considerations? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Haven't solved the problem I'm having yet, but I think it's really a bug |
|
This is because the |
|
Debugging reveals that |
|
I tried to keep startup ordering the same, but who knows 🙈 |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I don't think the startup order between extensions was ever guaranteed, if no dependencies were set. |
Yes, this dependency is really more of a headache |
|
There seems to be a problem here, it has to do with #15793 When src/OrchardCore.Modules/OrchardCore.Contents/Deployment/ExportContentToDeploymentTarget/ExportContentToDeploymentTargetMigrations.cs |
| "OrchardCore Deployment", | ||
| "OrchardCore.Deployment.Remote", | ||
| "OrchardCore.Contents.Deployment.AddToDeploymentPlan", | ||
| "OrchardCore.Contents.Deployment.Download", | ||
| "OrchardCore.Contents.Deployment.ExportContentToDeploymentTarget", |
There was a problem hiding this comment.
After changing the blog recipe to this, using the blog recipe when adding tenants will reproduce the problem
@gvkries
There was a problem hiding this comment.
Ok, thanks. I think I understand the problem. Give me a sec...
|
I think a quick fix for the problem is to use the last feature for migrations, instead of the first. But we may need a way to explicitly force a type like migrations to only have a single feature assigned. Try the following changes to the In line 92, use the last feature: public async Task<IEnumerable<string>> GetFeaturesThatNeedUpdateAsync()
{
var currentVersions = (await GetDataMigrationRecordAsync()).DataMigrations
.ToDictionary(r => r.DataMigrationClass);
var outOfDateMigrations = _dataMigrations.Where(dataMigration =>
{
Records.DataMigration record;
if (currentVersions.TryGetValue(dataMigration.GetType().FullName, out record) && record.Version.HasValue)
{
return CreateUpgradeLookupTable(dataMigration).ContainsKey(record.Version.Value);
}
return GetMethod(dataMigration, "Create") != null;
});
return outOfDateMigrations.Select(m => _typeFeatureProvider.GetFeaturesForDependency(m.GetType()).Last().Id).ToArray();
}And on line 279 as well: private IDataMigration[] GetDataMigrations(string featureId)
{
var migrations = _dataMigrations
.Where(dm => _typeFeatureProvider.GetFeaturesForDependency(dm.GetType()).Last().Id == featureId)
.ToArray();
return migrations;
}I will think of a better way to solve this, e.g. some marker interface to force types to have a single feature only. |
It worked! 🎉🎉 Thank you ! |
|
Here's the recipe that went wrong, and I'm now going to restore the blog recipe change. blog.recipe.jsonThe full recipe is too long, only the `feature` step have been taken.{
"steps": [
{
"name": "feature",
"enable": [
// SaaS
"OrchardCore.HomeRoute",
"OrchardCore.Admin",
"OrchardCore.Diagnostics",
"OrchardCore.DynamicCache",
"OrchardCore.Features",
"OrchardCore.Navigation",
"OrchardCore.Recipes",
"OrchardCore.Resources",
"OrchardCore.Roles",
"OrchardCore.Security",
"OrchardCore.Settings",
"OrchardCore.Themes",
"OrchardCore.Users",
// Content Management
"OrchardCore.Alias",
"OrchardCore.AdminMenu",
"OrchardCore.Autoroute",
"OrchardCore.Html",
"OrchardCore.ContentFields",
"OrchardCore.ContentPreview",
"OrchardCore.Contents",
"OrchardCore Deployment",
"OrchardCore.Deployment.Remote",
// start
"OrchardCore.Contents.Deployment.AddToDeploymentPlan",
"OrchardCore.Contents.Deployment.Download",
"OrchardCore.Contents.Deployment.ExportContentToDeploymentTarget",
// end
"OrchardCore.ContentTypes",
"OrchardCore.CustomSettings",
"OrchardCore.Feeds",
"OrchardCore.Flows",
"OrchardCore.Indexing",
"OrchardCore.Layers",
"OrchardCore.Lists",
"OrchardCore.Markdown",
"OrchardCore.Media",
"OrchardCore.Menu",
"OrchardCore.Placements",
"OrchardCore.Queries",
"OrchardCore.Queries.Sql",
"OrchardCore.Shortcodes.Templates",
"OrchardCore.Rules",
"OrchardCore.Taxonomies",
"OrchardCore.Title",
"OrchardCore.Templates",
"OrchardCore.Widgets",
// Themes
"TheBlogTheme",
"TheAdmin",
"SafeMode"
]
}
]
}
|
|
I have created #16257 with a different approach for fixing this problem. Please check it out, if you don't mind. |
…yzx86/OrchardCore into TypeUpdateConcurrencyException
|
I reverted some logic changes to the |
hyzx86
changed the title
hyzx86
changed the title
|
Let's wait until #16257 is merged to see if you can still repro the issue. I'd rather not change this code unless there are repro steps. |
I've tested it and it doesn't reproduce the problem. |
|
Thanks @hyzx86 for all your help! |
relate ：#15989 (comment)
relate: #16214