Optimize IndexingContentHandlers perf #13721

hyzx86 · 2023-05-20T09:46:05Z

try to fix #13071

Avoid retrieving content from the database multiple times.

Summary by CodeRabbit

New Features
- Enhanced content management capabilities with improved handling and indexing logic across different contexts such as publishing, updating, and creating content items.
Refactor
- Streamlined content indexing processes for Lucene, Azure AI, and Elasticsearch to boost efficiency and clarity.
Tests
- Expanded test coverage to include new utilities and indexing functionalities.

hyzx86 · 2023-05-20T15:07:29Z

I haven't changed the rebuildindex function yet, so let's see if I got it right here

Skrypt · 2023-05-23T04:44:42Z

I'm surprised it is broken. Will test it and analyze the PR.

github-actions · 2024-01-29T13:28:41Z

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

Piedone · 2024-03-21T21:14:33Z

Do you want to get back to this, @Skrypt?

Skrypt · 2024-03-22T00:24:51Z

Index Latest option is working on main branch. No issue there. I think that this PR will break it though. It may fix performance issues. We need to make sure that the PreviousItem and ContentItem from the Context is never altered.

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Handler/LuceneIndexingContentHandler.cs

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Services/LuceneIndexManager.cs

optimizing it

Add IsPublishing flag on PublishContentContext to handle when unpublishing

Skrypt

I think we need unit tests to make sure all scenarios work with the DefaultContentManager. Because, while testing this I needed to also fix the use case when we unpublish a content item which was not removing the content item in the index. I'm approving, but we need unit tests.

Piedone · 2024-04-25T18:30:48Z

Does this still need triage, @Skrypt? It seems it was actually triaged, with an all-start reviewer team being there.

Skrypt · 2024-04-29T18:47:31Z

I believe it still needs proper unit tests. If we want to merge then we need to at least create a task/ticket/issue about Lucene Unit Tests.

Piedone · 2024-04-29T20:15:41Z

What do we need the triage for, exactly? Because then we don't merge the PR until it has unit tests.

Skrypt · 2024-04-30T01:01:35Z

Ok, so no merge untill we have proper unit tests just like we said before.

hishamco · 2024-05-01T05:40:43Z

@hyzx86 can you write a unit test for this to finalize this PR

Let me know if you need help

hyzx86 · 2024-05-02T07:11:02Z

@hishamco, @Skrypt I need help ! 😅

Do we currently have a way to check that all indexing tasks in the system have been completed?
I noticed that there is a table called IndexingTask.
Can we check this table to make sure that all tasks have been completed?

https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Modules/OrchardCore.Indexing/IndexingTaskManager.cs

But I noticed that will always be some data in this table, I don't know if it is because the unit tests will not wait for
IndexingTaskManager.FlushAsync completes the reason, I will go to test

hishamco · 2024-05-02T07:15:24Z

As I looked at the code the FlushAsync() should handle this

I don't know if it is because the unit tests will not wait for
IndexingTaskManager.FlushAsync completes the reason,

Which unit test do you refer to?

…into lucenePublish

coderabbitai

Actionable comments posted: 3

Out of diff range and nitpick comments (1)

test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs (1)

Line range hint 1-59: Ensure consistent use of namespaces and imports.

The file includes multiple namespaces and using directives. Ensure that all these are necessary for the current test implementations to avoid unnecessary dependencies and to keep the code clean and maintainable.

test/OrchardCore.Tests/Utilities/TimeoutTaskRunner.cs

test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs

hyzx86 · 2024-05-02T07:58:45Z

test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs

+                {
+                    Name = "CheckIndexingTask",
+                    Template = "Select Count(1) as Total from IndexingTask"
+                };


@hishamco , Here ,
Looks like I found another Bug, and here it will never work because of syntax errors

OrchardCore/src/OrchardCore.Modules/OrchardCore.Indexing/IndexingTaskManager.cs

Lines 133 to 141 in c967b5d

var deleteCmd = $"delete from {dialect.QuoteForTableName(table, _store.Configuration.Schema)} where {dialect.QuoteForColumnName("ContentItemId")} {dialect.InOperator("@Ids")};";

do

{

var pageOfIds = ids.Take(pageSize).ToArray();

if (pageOfIds.Length > 0)

{

await transaction.Connection.ExecuteAsync(deleteCmd, new { Ids = pageOfIds }, transaction);

After I adjusted them... The data is still not deleted
ado.net directly use string collections as arguments in the in expressions?

Apparently not, the logic here is only to prevent repeated insertions, and there is no code to clean up the index task after the task is completed 😢

hyzx86 · 2024-05-02T11:42:19Z

test/OrchardCore.Tests/Apis/Lucene/LuceneQueryTests.cs

+
+                //await contentManager.CreateAsync(firstContent);
+                //var contentFromDb = await contentManager.GetAsync(firstContent.ContentItemId);
+                //Assert.NotNull(contentFromDb); faild


@Skrypt
Our content item operation is a bit complicated~~

Hi @Piedone ，need your help , I don't know how to do this unit test..
#15601 (comment)

What's your question, specifically?

Just as @Skrypt mentioned in this issue at #15601 ,
I understand that we should verify the impact on indexing functionality by separately testing Create, Update, Draft, and Publish actions with ContentManager.

However, based on the current design of DefaultContentManager which requires calling a sequence of methods to save data to the database and trigger Lucene indexing, it seems difficult if not impossible to isolate tests for just the Create and Update actions.

All that aside, am I currently unit testing the right way?

You don't need to test DefaultContentManager or have a full indexing test. Rather, unit tests on LuceneIndexingContentHandler with mocks is sufficient. I.e, the tests need to check that LuceneIndexingContentHandler behaves correctly based on what it calls on the mocks.

If you search for "mock.setup" and "mock.verify" in the codebase, you'll see how this works. A lot of tests do something similar.

…into lucenePublish

github-actions · 2024-05-22T00:30:08Z

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

github-actions · 2024-07-21T00:57:26Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

github-actions · 2024-08-05T04:40:24Z

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

avoid retrieving content from the database multiple times

6301402

hyzx86 mentioned this pull request May 20, 2023

Achieve the most efficient data import #13071

Open

Fixed multiple versions of Lucene Index storage issues

a31de04

hyzx86 changed the title ~~avoid retrieving content from the database multiple times~~ fix LuceneIndexingContentHandler Problems May 20, 2023

hishamco requested a review from Skrypt May 20, 2023 12:54

github-actions bot added the merge conflict label Jan 29, 2024

Merge branch 'main' into lucenePublish

78032d1

github-actions bot removed the merge conflict label Jan 29, 2024

Skrypt added the Lucene label Mar 22, 2024

Skrypt reviewed Mar 22, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Handler/LuceneIndexingContentHandler.cs Outdated Show resolved Hide resolved

Skrypt reviewed Mar 22, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Handler/LuceneIndexingContentHandler.cs Show resolved Hide resolved

Skrypt reviewed Mar 22, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Handler/LuceneIndexingContentHandler.cs Outdated Show resolved Hide resolved

Skrypt reviewed Mar 22, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Search.Lucene/Services/LuceneIndexManager.cs Outdated Show resolved Hide resolved

Skrypt added 2 commits March 21, 2024 22:13

Merge branch 'main' into pr/13721

26eb712

Format and typo

383704c

Skrypt previously approved these changes Mar 22, 2024

View reviewed changes

Skrypt changed the title ~~fix LuceneIndexingContentHandler Problems~~ Optimize LuceneIndexingContentHandler perf Mar 22, 2024

Optimize code

d2fbdcb

Use Context assigned contentItem

d3944c0

Add IsPublishing flag on PublishContentContext to handle when unpublishing

Skrypt approved these changes Mar 22, 2024

View reviewed changes

Skrypt requested a review from sebastienros March 22, 2024 04:05

Skrypt added needs triage Needs Unit Test(s) Unit Tests or Functional Tests are required labels Mar 22, 2024

Fix when saving draft

88ac104

Skrypt removed the needs triage label Apr 30, 2024

Merge branch 'main' into lucenePublish

98e694e

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into lucenePublish

5fe344e

hyzx86 added 2 commits May 2, 2024 15:50

FlushAsync error

f2bfa57

Merge branch 'lucenePublish' of https://github.com/hyzx86/OrchardCore …

36c7674

…into lucenePublish

coderabbitai bot reviewed May 2, 2024

View reviewed changes

hyzx86 marked this pull request as draft May 2, 2024 07:56

hyzx86 commented May 2, 2024

View reviewed changes

hyzx86 added 4 commits May 2, 2024 16:17

fix Flush

eb7ab7c

restore FlushAsync method

759c477

update unit test

11c76ad

add ContentManager Test

c0075be

hyzx86 commented May 2, 2024

View reviewed changes

hyzx86 added 3 commits May 2, 2024 22:51

Merge branch 'main' into lucenePublish

1af75f6

update RunTask

28e9928

Merge branch 'lucenePublish' of https://github.com/hyzx86/OrchardCore …

a894ed5

…into lucenePublish

github-actions bot added the merge conflict label May 22, 2024

github-actions bot added the stale label Jul 21, 2024

github-actions bot closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize IndexingContentHandlers perf #13721

Optimize IndexingContentHandlers perf #13721

hyzx86 commented May 20, 2023 •

edited by coderabbitai bot

Loading

hyzx86 commented May 20, 2023

Skrypt commented May 23, 2023

github-actions bot commented Jan 29, 2024

Piedone commented Mar 21, 2024

Skrypt commented Mar 22, 2024 •

edited

Loading

Skrypt left a comment

Piedone commented Apr 25, 2024

Skrypt commented Apr 29, 2024

Piedone commented Apr 29, 2024

Skrypt commented Apr 30, 2024

hishamco commented May 1, 2024

This comment was marked as outdated.

hyzx86 commented May 2, 2024

hishamco commented May 2, 2024

coderabbitai bot left a comment

hyzx86 May 2, 2024

hyzx86 May 2, 2024 •

edited

Loading

hyzx86 May 2, 2024

hyzx86 May 2, 2024

hyzx86 May 2, 2024 •

edited

Loading

Piedone May 2, 2024

hyzx86 May 2, 2024

Piedone May 3, 2024

github-actions bot commented May 22, 2024

github-actions bot commented Jul 21, 2024

github-actions bot commented Aug 5, 2024

	var deleteCmd = $"delete from {dialect.QuoteForTableName(table, _store.Configuration.Schema)} where {dialect.QuoteForColumnName("ContentItemId")} {dialect.InOperator("@Ids")};";

	do
	{
	var pageOfIds = ids.Take(pageSize).ToArray();

	if (pageOfIds.Length > 0)
	{
	await transaction.Connection.ExecuteAsync(deleteCmd, new { Ids = pageOfIds }, transaction);

Optimize IndexingContentHandlers perf #13721

Optimize IndexingContentHandlers perf #13721

Conversation

hyzx86 commented May 20, 2023 • edited by coderabbitai bot Loading

Summary by CodeRabbit

hyzx86 commented May 20, 2023

Skrypt commented May 23, 2023

github-actions bot commented Jan 29, 2024

Piedone commented Mar 21, 2024

Skrypt commented Mar 22, 2024 • edited Loading

Skrypt left a comment

Choose a reason for hiding this comment

Piedone commented Apr 25, 2024

Skrypt commented Apr 29, 2024

Piedone commented Apr 29, 2024

Skrypt commented Apr 30, 2024

hishamco commented May 1, 2024

This comment was marked as outdated.

hyzx86 commented May 2, 2024

hishamco commented May 2, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

hyzx86 May 2, 2024

Choose a reason for hiding this comment

hyzx86 May 2, 2024 • edited Loading

Choose a reason for hiding this comment

hyzx86 May 2, 2024

Choose a reason for hiding this comment

hyzx86 May 2, 2024

Choose a reason for hiding this comment

hyzx86 May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Piedone May 2, 2024

Choose a reason for hiding this comment

hyzx86 May 2, 2024

Choose a reason for hiding this comment

Piedone May 3, 2024

Choose a reason for hiding this comment

github-actions bot commented May 22, 2024

github-actions bot commented Jul 21, 2024

github-actions bot commented Aug 5, 2024

hyzx86 commented May 20, 2023 •

edited by coderabbitai bot

Loading

Skrypt commented Mar 22, 2024 •

edited

Loading

hyzx86 May 2, 2024 •

edited

Loading

hyzx86 May 2, 2024 •

edited

Loading