Optimizing Sitemap Creation with Batched Content Items

[MikeAlhayek]

Fix #16616

[Piedone]

We can't really verify this solving the problem until we can reproduce it. Did you generate items or something?

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[MikeAlhayek]

We can't really verify this solving the problem until we can reproduce it. Did you generate items or something?

@Piedone Hopefully @MikeKry can try to reproduce against his data. I don't want to have to create 100k content items here.

[gvkries]

We can't really verify this solving the problem until we can reproduce it. Did you generate items or something?

Even if we did not reproduce it on our own, this is clearly an issue. Loading the whole DB in memory is never a good idea, IMHO 😉 And in theory it is easy enough to reproduce, when generating a lot of content items.

[MikeKry]

@MikeAlhayek

I have tried it on same data - oc 1.8.3 vs main vs this branch (ma/batch-sitemap-items) and I confirm 👏 that it is indeed fixed.

Memory allocation is just around 100MB when generating sitemap and it seems that memory is released after sitemap is generated.

But regarding branch 1.8.3, it seems there are some nugets that are marked as security risk, maybe it would be worth releasing 1.8.4 to get rid of them.. (if possible with this fix also?)

[MikeAlhayek]

@MikeKry thanks for conforming. This can't be back ported into 1.8 because it has a breaking change. So if you want this change, you'll have to upgrade to 2.0. In fact, everyone should consider migrating to 2.0 since it has lots of improvements specially around performance and APIs.

@gvkries @Piedone this is the confirmation that it indeed solves the problem :)

[sebastienros]

@MikeKry we should not ship new versions when transitive dependencies have vulnerabilities. But when we ship ensure that we are using the latest. If you detect a dependency has a vulnerability then you can update it from your project references to fix it. The other advantage is that you don't have to wait for orchard to get the fix. We may change our stance based on the type of vulnerability though. Maybe we should have a page describing this process, and maybe we could keep it up to date with the changes to apply to the csproj to get all the versions safe without the need to do a new release. This should be easy to detect since nuget lists them on every build now.

[Piedone]

I won't be able to look into this in the next few days, so up to everyone else here :).

@gvkries theory is one thing, but it's definitely not the only one we should rely on when trying to fix issues, we also need to test the actual scenario (which might bring further surprises). Note that I'm not arguing with the validity bug report

[gvkries]

@Piedone I just think we should not wait for specific ways to reproduce something, if we already acknowledged a problem. I'm not arguing against testing :)

[MikeAlhayek]

@MikeKry if we take this PR as-is, we should be able to backport into 1.8 and release 1.8.4 at the same time we release 2.0.0.

[MikeAlhayek]

/backport to release/1.8.3

[github-actions]

Started backporting to release/1.8.3: https://github.com/OrchardCMS/OrchardCore/actions/runs/10640307448

[github-actions]

@MikeAlhayek backporting to release/1.8.3 failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Batch the process of building sitemaps
Using index info to reconstruct a base tree...
M	src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/LocalizedContentItemsQueryProvider.cs
M	src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/SitemapUrlHrefLangExtendedMetadataProvider.cs
M	src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/ContentTypesSitemapSourceBuilder.cs
M	src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/DefaultContentItemsQueryProvider.cs
M	src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ContentItemsQueryContext.cs
M	src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/IContentItemsQueryProvider.cs
M	src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ISitemapContentItemExtendedMetadataProvider.cs
A	src/docs/releases/2.0.0.md
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/docs/releases/2.0.0.md deleted in HEAD and modified in Batch the process of building sitemaps. Version Batch the process of building sitemaps of src/docs/releases/2.0.0.md left in tree.
Auto-merging src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ISitemapContentItemExtendedMetadataProvider.cs
CONFLICT (content): Merge conflict in src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ISitemapContentItemExtendedMetadataProvider.cs
Auto-merging src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/IContentItemsQueryProvider.cs
CONFLICT (content): Merge conflict in src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/IContentItemsQueryProvider.cs
Auto-merging src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ContentItemsQueryContext.cs
CONFLICT (content): Merge conflict in src/OrchardCore/OrchardCore.Sitemaps.Abstractions/Builders/ContentItemsQueryContext.cs
Auto-merging src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/DefaultContentItemsQueryProvider.cs
CONFLICT (content): Merge conflict in src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/DefaultContentItemsQueryProvider.cs
Auto-merging src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/ContentTypesSitemapSourceBuilder.cs
CONFLICT (content): Merge conflict in src/OrchardCore.Modules/OrchardCore.Contents/Sitemaps/ContentTypesSitemapSourceBuilder.cs
Auto-merging src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/SitemapUrlHrefLangExtendedMetadataProvider.cs
CONFLICT (content): Merge conflict in src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/SitemapUrlHrefLangExtendedMetadataProvider.cs
Auto-merging src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/LocalizedContentItemsQueryProvider.cs
CONFLICT (content): Merge conflict in src/OrchardCore.Modules/OrchardCore.ContentLocalization/Sitemaps/LocalizedContentItemsQueryProvider.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Batch the process of building sitemaps
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@MikeAlhayek an error occurred while backporting to release/1.8.3, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[sebastienros]

Is it even worth backporting since there will be a new release soon with this improvement? Why this more that any other bug (not advocating to backport all bugs).

I think we should backport security vulnerabilities, critical bugs (a feature can't be used reasonably) and regressions.

This "could" fall in the "critical bugs" since in @MikeKry 's case the feature doesn't work with their amount of data. I almost thing we should update the backport template to explain why this has to be backported, with the reasons I mentioned above (note this is how we do it in aspnet and dotnet repos).