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

Added cron to remove old index events for manual indexers #2474

Conversation

tobihille
Copy link
Contributor

Description (*)

As discussed in #199 I created a new PR which tackles the problem from another direction: Instead of completely omitting to write the index event to the database I created a cron that will delete these events if they are not processed.
This way we maintain BC and avoid filling the DB with garbage that might not be needed.

Related Pull Requests

#199

Manual testing scenarios (*)

  1. Set index mode to manual in admin
  2. Save a product
  3. Execute select count(*) from index_event
  4. Wait 24 hours
  5. Run php cron.php (I executed the job from Aoe_Scheduler but the result should be the same)
  6. Execute select count(*) from index_event

Questions or comments

I would gladly discuss the introduction of Mage_Index_Model_Observer::OLD_INDEX_EVENT_THRESHOLD_SECONDS, in my opinion this is a quick-and-dirty way to start this PR but I do not expect it to be in the final merge.
I would prefer a config but Mage_Index does not have a system.xml - so where would be good place for it?
If we add a config: what should be the default value?
Is one day a sufficient threshold for e.g. the Hackathon_AsyncIndex (on my workplace we index every day even if we have over 100.000 products and thousand categories)?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: Index Relates to Mage_Index label Aug 23, 2022
@Sdfendor
Copy link
Contributor

Is the introduction of a cronjob a reason to bump the version number of the module 🤔?

@fballiano
Copy link
Contributor

Is the introduction of a cronjob a reason to bump the version number of the module 🤔?

I'd bump only if there's a new install script to modify the database

@Sdfendor
Copy link
Contributor

Fair point after all its one of the main reason to have versions of OpenMage modules to begin with.

app/code/core/Mage/Index/Model/Observer.php Outdated Show resolved Hide resolved
app/code/core/Mage/Index/Model/Observer.php Outdated Show resolved Hide resolved
@fballiano fballiano force-pushed the cron-delete-old-index-events-on-manual-index branch from 14f0afb to 3b7af67 Compare August 24, 2022 07:21
@fballiano fballiano changed the title feat(index): added cron which removes old index events for manual indexers Added cron to remove old index events for manual indexers Mar 27, 2023
@fballiano
Copy link
Contributor

I think this one should be merged

fballiano
fballiano previously approved these changes Mar 27, 2023
@fballiano fballiano requested review from elidrissidev and kiatng and removed request for Sdfendor March 27, 2023 23:27
empiricompany
empiricompany previously approved these changes May 9, 2023
@fballiano fballiano requested a review from Sdfendor May 9, 2023 17:42
Sdfendor
Sdfendor previously approved these changes May 9, 2023
@fballiano
Copy link
Contributor

we could merge because it's 1 green + 2 gray but I notice now that it's the wrong branch, can't merge on 1.9.4.x, we've to rebase on main

@fballiano fballiano changed the base branch from 1.9.4.x to main May 9, 2023 18:06
@fballiano fballiano dismissed stale reviews from Sdfendor and empiricompany May 9, 2023 18:06

The base branch was changed.

@fballiano fballiano dismissed their stale review May 9, 2023 18:06

The base branch was changed.

@fballiano fballiano changed the base branch from main to 1.9.4.x May 9, 2023 18:06
@fballiano fballiano force-pushed the cron-delete-old-index-events-on-manual-index branch from c17e022 to dd5bdf9 Compare May 9, 2023 18:30
@fballiano fballiano changed the base branch from 1.9.4.x to main May 9, 2023 18:31
@fballiano
Copy link
Contributor

I'm merging because it was already approved (1green + 2gray checks) but I had to rebase (checked and the diff is exactly the same)

@fballiano fballiano merged commit 460ce65 into OpenMage:main May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants