putyourlightson / craft-blitz

Intelligent static page caching for creating lightning-fast sites with Craft CMS.
https://putyourlightson.com/plugins/blitz
Other
149 stars 36 forks source link

Use of Redis yields "job priority is not supported in the driver" error #201

Closed martinhellwagner closed 4 years ago

martinhellwagner commented 4 years ago

Describe the bug

We're using Redis for our Craft CMS queue. On top of it, we're using Blitz to cache certain sites that receive lots of traffic. However, using Blitz with Redis always results in the error message "job priority is not supported in the driver" when trying to upload an image or create an entry. It doesn't matter whether or not we're using Blitz File Storage or Yii Cache Storage, and whether or not we're moving cache to Redis – always the same error.

To reproduce

Steps to reproduce the behaviour:

  1. Install Redis and Blitz on Craft CMS.
  2. Try to upload an asset or create an entry.

Expected behaviour

Upload / creation should be fine.

Screenshots

Screenshot 2020-04-02 at 17 17 51

Versions

martinhellwagner commented 4 years ago

Seems like when I remove the call where the priority of the Craft queue is set in the source code, then it's working fine. Would it be an idea to set the priority to -1 or something like that when you don't want to (or can't) have priority on your queue, and check it in the source code like this?

if ($priority >= 0) {
  $queue->priority($priority);
}
bencroker commented 4 years ago

Hmm, are you using yii\queue\redis\Queue? It extends yii\queue\Queue which contains the priority method, so should still work.

martinhellwagner commented 4 years ago

Yes, this is what our config/app.php looks like:

    'components' => [
        // Initialize Redis component
        'redis' => [
            'class' => yii\redis\Connection::class,
            'hostname' => 'localhost',
            'port' => 6379,
            'password' => null,
        ],
        // Use queue with Redis
        'queue' => [
            'class' => yii\queue\redis\Queue::class,
            'redis' => 'redis',
            'channel' => 'queue',
        ],
    ],
martinhellwagner commented 4 years ago

Apart from that, we're requiring the Yii Redis Component in the composer.json like this:

"yiisoft/yii2-redis": "^2.0"

Our servers are provisioned by Forge, so Redis is baked in there. Is there anything we are doing wrong?

martinhellwagner commented 4 years ago

I stumbled upon this issue, where it's claimed that Redis hasn't implemented job priority: https://github.com/yiisoft/yii2-queue/issues/205

bencroker commented 4 years ago

Thanks, will look into adding a workaround soon.

martinhellwagner commented 4 years ago

Thank you! 🙂

Since we need the functionality pretty soon, do you see any problems in forking the plugin for now and commenting out the two lines in the source code where priority is set?

bencroker commented 4 years ago

Fixed in https://github.com/putyourlightson/craft-blitz/commit/6f6c1158b5625c5f3770f7b17b0f64d8a53f8d8d on the develop branch, so you can use that until the next release.

martinhellwagner commented 4 years ago

Wonderful, cheers! 😍

martinhellwagner commented 4 years ago

@bencroker

Just installed the fix and tested it: unfortunately, this doesn't solve the problem, as Craft queue indeed has a priority – just the Redis queue doesn't.

Is it possible to check in the if whether or not Redis is used as a queue driver – or maybe actually use a parameter in the config file to do so?

bencroker commented 4 years ago

Yeah, you're right, the Redis queue just throws an error if a priority is set. https://github.com/yiisoft/yii2-queue/blob/2.3.0/src/drivers/redis/Queue.php#L190-L192

Will work on another fix for this.

bencroker commented 4 years ago

Haven't had a chance to test, but the priority is now not set when using a Redis queue in https://github.com/putyourlightson/craft-blitz/commit/042a9f308f46ef235f6db06a2e5485fc9ef4d9be.

martinhellwagner commented 4 years ago

Will test right away!

martinhellwagner commented 4 years ago

Yes, that looks very good! Thanks again for the quick fixes! 💯

martinhellwagner commented 4 years ago

One last question: when we deploy a new release, we naturally want the Blitz caches to be in that release as well. That's why we have the CLI command ./craft blitz/cache/refresh in our pipeline. But when the command is called in the pipeline (or directly in the Craft CLI), there's no cache/blitz folder to be found in the new release.

I'm getting the situation of Screenshot 1 (left: new release, right: old release) whereas I'd be expecting the scenario of Screenshot 2 after the CLI command – which I'm only getting after I've visited the respective cached page.

Screenshot 2020-04-03 at 12 01 54

Screenshot 2020-04-03 at 12 03 47

martinhellwagner commented 4 years ago

Seems like ./craft blitz/cache/warm works fine in the pipeline and in the CLI. 🤔

martinhellwagner commented 4 years ago

By the way, I think also the Craft guys fixed the issue on their side: https://github.com/craftcms/cms/issues/5876

bencroker commented 4 years ago

They did, and I've since followed suit with catching an exception in https://github.com/putyourlightson/craft-blitz/commit/167f277858e848e68f2e4dbcd9f9c22fe57f7a57.

martinhellwagner commented 4 years ago

@bencroker

Just realized that not only the ./craft blitz/cache/refresh CLI command works differently than expected (see above), but also the ./craft blitz/cache/refresh-expired seems to be crashing when the queue is in Redis:

Screenshot 2020-04-04 at 21 01 39

martinhellwagner commented 4 years ago

Also, it seems our cached HTML files get deleted, even though we don't have a specific cache duration set (from the config file, I see that the default value 0 means unlimited caching). Is it a good idea to activate the "Warm Cache Automatically" parameter in the settings?

bencroker commented 4 years ago

Ok, hopefully https://github.com/putyourlightson/craft-blitz/commit/dee62a964874227de36e269a592acd4540b27c4e will fix the error in the console.

Just realized that not only the ./craft blitz/cache/refresh CLI command works differently than expected

The refresh command will clear, flush and purge the cache, and then warm and deploy it only if the Warm Cache Automatically setting is enabled.

martinhellwagner commented 4 years ago

Looks good, thanks! 😍

martinhellwagner commented 4 years ago

@bencroker

Is there any setting that clears the cached HTML files when the expiry date is not specifically set (and thus infinite) and the "Clear Cache Automatically" parameter disabled? Our caches are rebuilt periodically whereas they should always have the same cached file.

Maybe Craft caching is still interfering here?

martinhellwagner commented 4 years ago

This could also be related to https://github.com/putyourlightson/craft-blitz/issues/70?

bencroker commented 4 years ago

Have you disabled Craft caching by setting enableTemplateCaching to false in the general config?

What action triggers a cache refresh? No files should be deleted if Clear Cache Automatically is disabled, until refresh-expired is called. If this persists then please open a new isssue with some details and I'll help troubleshoot there.

martinhellwagner commented 4 years ago

No, enableTemplateCaching is set to true in our Stage and Production environments. Reason for this is that we're actually not caching all the pages with Blitz (just the most visited ones) and rely on Craft caching for the others. Creating and saving an entry both triggers the Blitz Cache rebuild.

Guess the "dual" caching strategy is the problem?

bencroker commented 4 years ago

While using both caching systems together can cause issues, it shouldn't lead to pages cached by Blitz being cleared unexpectedly.

martinhellwagner commented 4 years ago

Just tried setting the parameter to false and creating an entry again – still happens. I'll research some more, but I might have to open an issue about this. 🤔

john-henry commented 2 years ago

Im getting this now when I upload an Asset. I have narrowed it down to Blitz

Craft 3.7.39 Blitz 3.12.2

I only installed Redis this morning and all of that works fine

Here is Sentry error https://sentry.io/share/issue/e483de1948d04ac4bdf28030f73f9990/

I do use enableTemplateCaching set to true but I have tested on or off and same issue uploading

john-henry commented 2 years ago

Now Im not even sure if any Blitz tasks have made the new queue today. Am I missing some config option?

@bencroker This is on saving entries too. You want me to make a new issue or its ok to live here?

bencroker commented 2 years ago

This issue is 2 years old, new issue please.