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

[FEATURE] Add flag to site config to skip DataHandler hooks #4109

Open
wants to merge 1 commit into
base: release-12.0.x
Choose a base branch
from

Conversation

kitzberger
Copy link
Contributor

@kitzberger kitzberger commented Jul 16, 2024

What this pr does

Currently the 2 hooks of EXT:solr into DataHandler are fired for all sites, their pages and records. This is time consuming. Especially the RecordMonitor is taking up to 10s to do its thing.

How to test

Add the flag solr_skip_hooks to a site config file and edit/save one of it's pages. Both GarbageCollector and RecordMonitor should skip their processDatamap_afterDatabaseOperations method.

Fixes: #4108


Maintainers Notes:

  • Don't merge into release-12.0.x

@dkd-kaehm
Copy link
Collaborator

dkd-kaehm commented Jul 17, 2024

Thanks for that feature.

Why not using the "Enable Solr for this site" checkbox from Site-Config?

grafik

What happens with mounted pages between different sites?


I'll change the base and target branch to release-12.0.x to be able to run the tests. Currently the main branch not stable enough for new features. We'll switch back to main, if that feature should be merged.
Please use git reset --hard origin/skip-hooks to work on that feature.

@dkd-kaehm dkd-kaehm changed the base branch from main to release-12.0.x July 17, 2024 07:02
@dkd-kaehm dkd-kaehm changed the title Add flag to site config to skip DataHandler hooks [FEATURE] Add flag to site config to skip DataHandler hooks Jul 17, 2024
@kitzberger
Copy link
Contributor Author

@dkd-kaehm, to be honest I don't know the implications behind that solr_enable_read flag.

Thing is, we are using EXT:solr for the site in question but not its indexing. That's why we needed a separate flag. Does this make sense? I not 100% sure ;-)

I've noticed that RecordMonitor checks on no_search_sub_entries being present in the rootline of the modified record. That's a neat feature but a bit magic too. I wouldn't assume (as an integrator) that this flag had any effect on the RecordMonitor. I'd rather disable the hook in the site config.

@dkd-kaehm
Copy link
Collaborator

@dkd-kaehm, to be honest I don't know the implications behind that solr_enable_read flag.

Thing is, we are using EXT:solr for the site in question but not its indexing. That's why we needed a separate flag. Does this make sense? I not 100% sure ;-)

I've noticed that RecordMonitor checks on no_search_sub_entries being present in the rootline of the modified record. That's a neat feature but a bit magic too. I wouldn't assume (as an integrator) that this flag had any effect on the RecordMonitor. I'd rather disable the hook in the site config.

OK, you see, there is another way with no_search_sub_entries, so why introducing the new setting?
Would you like to reuse the existing possibilities/settings and maybe add a hint in the docs, for example in FAQ?
Beside of that, there is a feature for your purpose: https://docs.typo3.org/p/apache-solr-for-typo3/solr/12.0/en-us/Configuration/Reference/ExtensionSettings.html#monitoringtype

I would propose to fix the existing things if they not working well, instead of introducing new settings.

@kitzberger
Copy link
Contributor Author

Beside of that, there is a feature for your purpose: https://docs.typo3.org/p/apache-solr-for-typo3/solr/12.0/en-us/Configuration/Reference/ExtensionSettings.html#monitoringtype

Thanks for pointing that out! (didn't know about that). But unfortunately that's a global option, right? I need a per site option. Hence my PR.

@dkd-kaehm
Copy link
Collaborator

dkd-kaehm commented Jul 17, 2024

Beside of that, there is a feature for your purpose: https://docs.typo3.org/p/apache-solr-for-typo3/solr/12.0/en-us/Configuration/Reference/ExtensionSettings.html#monitoringtype

Thanks for pointing that out! (didn't know about that). But unfortunately that's a global option, right? I need a per site option. Hence my PR.

This could be a option for that feature.
@dkd-friedrich Please join the discussion.

@dkd-friedrich
Copy link
Member

My first thought was also that it might be useful to be able to set the monitoringType per site. We are planning to add site information anyway to improve the maintenance of the event queue.

But if Solr connections are deactivated for a site, it makes sense to cancel the monitoring, but I'm not sure what effect this will have on mounted sites. I think there are some possible side effects to consider here.

@dkd-kaehm dkd-kaehm force-pushed the release-12.0.x branch 2 times, most recently from 3cf43b6 to 8f60143 Compare July 30, 2024 06:40
@kitzberger
Copy link
Contributor Author

I've come across another problem now: trying to save be_users is running into memory exhaustion. Since those records are "global" (pid=0) the pages.no_search_sub_entries setting cannot be used to deactivate those hooks.

Currently the 2 hooks of EXT:solr into DataHandler are fired for all sites, 
their pages and records. This is time consuming. 
Especially the RecordMonitor is taking up to 10s to do its thing.
Both GarbageCollector and RecordMonitor should skip their 
processDatamap_afterDatabaseOperations method, to avoid 
unneccessery processing for sites.

Fixes: TYPO3-Solr#4108
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.

[FEATURE] Feature flag to skip RecordMonitor and GarbageCollection
3 participants