-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Use HttpBackgroundJob instead of Deferred Tasks during search indexing #14063
base: main
Are you sure you want to change the base?
Conversation
I will review soon, maybe also in In the meantime I see one problem, e.g. when we publish an item from a BackgroundTask, in this context we can't trigger a BackgroundJob, we would need the same check done in the extension itself.
Need to check why unit tests fail. Also, will need to be reviewed by @Skrypt. Hmm, also when executing recipes, each step is executed in its own scope, a step may update content items, then a subsequent step may need these items to be already indexed, which would not be the case if we wait the end of the request in place of waiting the end of the step scope. Maybe this is why unit tests are failing, will think about it tomorrow. |
@hyzx86 Okay I fixed the unit tests by adding a delay in 3 places.
Not sure it's a common case to update an index and just after do a query that should rely on this index update, but at least it will break those that are using the same kind of unit tests. Hmm, finally maybe okay when executed through a recipe. But still the problem when for example we publish an item in a background task, we would need to check it (easy to do) and then still use the deferred task. OR at the end of a background task, in the same way we set an HttpContext, we could clear it (as aspnetcore does at the end of a request) and maybe the ActionContext too (as aspnetcore doesn't). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and it needs changes before it can be merged.
test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs
Outdated
Show resolved
Hide resolved
Is this something you'd like to revisit any time soon @hyzx86 or should we close? |
This should work after merging #13721 as it will no longer do a SQL query in the LuceneIndexingContentHandler. Though same logic needs to be replied in ElasticIndexingContentHandler and AzureAISearch one too. |
&& "new version".Equals(nodes[0]["displayText"].ToString()) | ||
&& "second content item display text".Equals(nodes[1]["displayText"].ToString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the strings to the right of the operator, and simply use ==
.
&& "second content item display text".Equals(nodes[1]["displayText"].ToString()) | ||
) | ||
{ | ||
Assert.True(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't see what you mean. Do I need to change the code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs
Outdated
Show resolved
Hide resolved
…ContentStepLuceneQueryTests.cs Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
While I pointed out some small issues, I'll only review this is Jasmin won't. |
@@ -81,6 +81,10 @@ public virtual async Task InitializeAsync() | |||
TenantName = tenantName; | |||
} | |||
|
|||
// Search indexes are no longer updated in a deferred task at the end of a shell scope | |||
// but in a background job after the http request, so they are not already up to date. | |||
await Task.Delay(2_000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can make the test flaky. Instead, explicitly wait for the process to finish, don't really on occasionally correct time.
See how you fixed this under another PR of yours when I pointed it out.
Also in RecentBlogPostsQueryTests
.
WalkthroughThe recent updates in OrchardCore involve transitioning indexing operations from executing at the end of a shell scope to a background job after an HTTP request. This change aims to boost performance by running the indexing process asynchronously. It impacts Lucene and Elasticsearch handlers, along with related test scenarios, ensuring indexing aligns with the new asynchronous execution model. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
test/OrchardCore.Tests/Apis/GraphQL/Queries/RecentBlogPostsQueryTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/ContentStepLuceneQueryTests.cs
Outdated
Show resolved
Hide resolved
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
test/OrchardCore.Tests/Utilities/TimeoutTaskRunner.cs (1)
14-14
: Reduce the delay interval or make it configurable.Currently, there is a hardcoded delay of 500 milliseconds in each iteration of the loop. This might not be optimal for all scenarios and could lead to unnecessary delays in tests. Consider making this delay configurable or reducing it to speed up the checks.
@Piedone , This PR is ready to review. 😊 |
This is meant to be applicable only after #13721, no? |
This comment was marked as off-topic.
This comment was marked as off-topic.
Okay, I did get the order wrong, but this PR I think is done. |
Please let me know when the other one is merged and we can get back to this. |
This pull request has merge conflicts. Please resolve those before requesting a review. |
relate: #10858 (comment)
Summary by CodeRabbit
New Features
Refactor
Tests