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

fix: made script which can create required directories, container and warmup application cache, so we can initialize applicaton without installing or updating it. #3870

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

vbyndych
Copy link
Contributor

@vbyndych vbyndych commented Aug 10, 2023

Goal

Make sure the cache directory exists before creating a service container file. This way we can ensure no error happen during later steps.

Please check the linked PR for more context but in a nutshell:

My idea is to create one more script similar to install or update which will take care of initializing the application during the new instance start-up. I decided not to put it inside the cache warmer itself because it is not his responsibility and in case we would change the pipeline later, we would still be able to use the application cache warmer without any modifications.

Changelog

  • fix: made script which can create required directories, container and warmup application cache, so we can initialize applicaton without installing or updating it.

How to test / use

  1. Install application
  2. Remove everything inside data directory
  3. Run the command php index.php oat\generis\scripts\tools\ContainerCacheWarmup

Linked PRs:

… warmup application cache, so we can initialize applicaton without installing or updating it.
@github-actions
Copy link

Version

Target Version 53.4.4
Last version 53.4.3

There are 0 BREAKING CHANGE, 0 feature, 1 fix

Copy link
Contributor

@gabrielfs7 gabrielfs7 left a comment

Choose a reason for hiding this comment

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

Thanks @vbyndych!

I am approving as I understand this code is needed for premium pipeline specific, but my recommendation is different:

  • We already have a command taoUpdate.php that does (or should do) the same thing.
  • Calling taoUpdate.php would produce the same result + generate also ACL, menu and other caches that we also need.
  • Generating these other caches mentioned above would lead us to even more speed and avoiding them to be generate on the first real customer requests.

So, I am approving, but I would definitely just use existing taoUpdate command instead as now we would have one more command to do part of the work.

Copy link
Contributor

@augustas augustas left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@vbyndych
Copy link
Contributor Author

@gabrielfs7 thank you so much for your advice. We have discussed it internally within the reliability team and have decided to give this script a try. There 2 main reasons for such decision:

  1. Premium workflow is a bit different from enterprise one and we use the dedicated instance to update the platform, so taoUpdate runs only on one separate instance and only there. Of course, nobody will be heart if we will add taoUpdate to every web server, but here we come to the second reason.
  2. I've done some tests with the latest version of tao premium and it seems that taoUpdate does not warm up enough cache to solve the cold start issue. For example in my local environment Items page took only 45ms to load after taoInit and 9 seconds to load after taoUpdate. The reason behind it is that taoUpdate apparently loads the bare minimum of the cache (container, menu, languages) but omits parts that cause significant load (like ACL, controller annotations, etc.). Just to give you a taste, here are the cache misses for both of the situations:
    cache_misses_init.log
    cache_misses_update.log

That's why we want to give it a try and see if this new approach can bring some benefits and potentially could solve our client's performance issues.

@vbyndych vbyndych merged commit 9b430b4 into develop Aug 11, 2023
4 checks passed
@vbyndych vbyndych deleted the fix/REL-1234/release-update branch August 11, 2023 16:49
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.

3 participants