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

Remove Lucene from built-in recipes (Lombiq Technologies: OCORE-84) #11328

Merged
merged 26 commits into from
Apr 13, 2022

Conversation

BanzragchUchral
Copy link
Contributor

Fixes #11203

@Piedone Piedone marked this pull request as draft March 7, 2022 20:56
@BanzragchUchral
Copy link
Contributor Author

Two tests don't work anymore because the base Blog recipe doesn't contain Lucene anymore. What should I do with them? Can I delete them?

@Piedone
Copy link
Member

Piedone commented Mar 8, 2022

The tests need to be kept but modify them that they can utilize the new factored out recipes.

src/docs/getting-started/starter-recipes.md Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Apis/Context/SiteContext.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Apis/Context/SiteContext.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Apis/Context/SiteContext.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Apis/Context/SiteContext.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Apis/Context/SiteStartup.cs Outdated Show resolved Hide resolved
@Piedone Piedone marked this pull request as ready for review April 11, 2022 19:47
@Piedone Piedone requested a review from agriffard as a code owner April 11, 2022 19:47
@Piedone Piedone requested a review from Skrypt April 11, 2022 19:47
@Piedone
Copy link
Member

Piedone commented Apr 11, 2022

Could you please also do a review now, @Skrypt?

Co-authored-by: Zoltán Lehóczky <[email protected]>
@Skrypt
Copy link
Contributor

Skrypt commented Apr 12, 2022

I will do it tomorrow.

@Piedone
Copy link
Member

Piedone commented Apr 12, 2022

Thanks!

@@ -44,6 +44,7 @@ public async Task ExecuteAsync(RecipeExecutionContext context)
{
luceneIndexSettings.Value.IndexName = luceneIndexSettings.Key;
await _luceneIndexingService.CreateIndexAsync(luceneIndexSettings.Value);
await _luceneIndexingService.ProcessContentItemsAsync(luceneIndexSettings.Key);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can cause a request timeout which is why it was not added. In this case, if the recipe fails to execute then executing it a second time would probably not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a fire and forget type of execution here. Start the indexing background task for example.

Copy link
Contributor

@Skrypt Skrypt left a comment

Choose a reason for hiding this comment

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

Looks good except the recipe step should not process the content items. Also, it may fail to index these Blog Posts since they are probably not added yet to the IndexingTask table.

@Skrypt
Copy link
Contributor

Skrypt commented Apr 13, 2022

Can you assert that the IndexingTask table has records before executing the recipes? Else, we will need a method to resync the already created content items in that table.

@BanzragchUchral
Copy link
Contributor Author

You mean before running the new Lucene recipe, right? I can see a few records here. I am not sure if this is what you wanted to see.

image

@Skrypt
Copy link
Contributor

Skrypt commented Apr 13, 2022

Ok, this works with TheBlog recipe because we enable the Lucene feature from the setup. But let's say we do the same but with the SaaS recipe then. I'm pretty sure that the IndexingTask table will not be there. So, if we create content items before enabling the Lucene feature then these content items will not be added to the IndexingTask table.

I'm ok to merge the current PR as is since this is a known issue that needs to be fixed. But we could fix it with a different PR.

@Piedone Piedone merged commit c2e1e39 into OrchardCMS:main Apr 13, 2022
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.

Remove Lucene from built-in recipes
3 participants