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

Add events before and after handling bulk imports #11532

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Apr 13, 2022

Scene:

  1. Process the data before bulk import
  2. Batch update dynamic SQL indexes after batch import

I am using FreeSql as an ORM tool that supports dictionary type data insertion/update (including batch mode)
http://www.freesql.net/en/guide/insert-or-update.html#_2-ifreesql-insertorupdatedict

var dic = new Dictionary<string, object>();
dic.Add("id", 1);
dic.Add("name", "xxxx");

fsql.InsertOrUpdateDict(dic).AsTable("table1").WherePrimary("id").ExecuteAffrows();

related: #8869

@Skrypt
Copy link
Contributor

Skrypt commented Apr 13, 2022

I like it.

@ns8482e
Copy link
Contributor

ns8482e commented Apr 13, 2022

Isn't Importing runs before import and Imported runs after import already?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Apr 14, 2022

Isn't Importing runs before import and Imported runs after import already?

Existing code is triggered one by one. What I want is complete pre - and post-import data
It is convenient to handle the large amount of data to be imported in batches.
For example, we can use it to perform refresh Lucene indexes

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jan 20, 2023

related #13071

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

You mention use case under https://github.com/OrchardCMS/OrchardCore/pull/11532/files#r1082149869 Wouldn't you add some to demonstrate how and why you'd use these new events?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Mar 14, 2024

link to #14630

Copy link
Contributor

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 and remove the "stale" label.

@github-actions github-actions bot added the stale label May 14, 2024
@github-actions github-actions bot removed the stale label May 15, 2024
@hyzx86 hyzx86 requested review from ns8482e and Piedone May 16, 2024 18:07
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

You didn't reply here: #11532 (review)

hyzx86 and others added 2 commits May 17, 2024 14:55
…dlers/IBulkContentHandler.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
…dlers/IBulkContentHandler.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
@hyzx86
Copy link
Contributor Author

hyzx86 commented May 17, 2024

You didn't reply here: #11532 (review)

Hmmm. In my scenario, I'm just trying to optimise the speed of the import because I've done a function that maps to a table based on the content type definition, and it doesn't work on its own, so it becomes a question of how to design the example

@Piedone
Copy link
Member

Piedone commented May 17, 2024

Aren't there use cases in OC itself?

Copy link
Contributor

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

@Piedone
Copy link
Member

Piedone commented Jun 16, 2024

@hyzx86 there's very little remaining here, please reply.

@MikeAlhayek
Copy link
Member

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 16, 2024

@Piedone , Sorry I've been busy lately and missed some information 😐
the github message notify are no longer prompted on the webpage after being read in the email. I tried to find the corresponding setting but couldn't

Aren't there use cases in OC itself?

I mean if I want to test this feature I need to add a default implementation of IBulkContentHandler
How can I inject this implementation in the unit test?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 16, 2024

Maybe I can test it this way?

 var cmanager =  (DefaultContentManager)contentManager;
 cmanager.BulkContentHandlers.add(new TestBulkContentHandler());
...

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

I mean, isn't there a use case in Orchard Core where you'd add an IBulkContentHandler implementation? There is no existing code in OC that would utilize this already? If not, then OK.

Copy link
Contributor

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

@hyzx86 hyzx86 requested a review from Piedone August 19, 2024 06:12
hyzx86 and others added 3 commits August 21, 2024 11:35
…nager.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
…BulkContentEventHandlerBase.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
@Piedone
Copy link
Member

Piedone commented Oct 14, 2024

You need to fix the build errors.

When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

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.

5 participants