PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

GromNaN · 2024-04-22T09:58:38Z

By using an UTCDateTime for the expiration field of cache and lock collections, we can create a TTL index.

// Create TTL index on cache collection
Cache::driver('mongodb')->createTTLIndex();

// Create TTL index on lock collection
Cache::driver('mongodb')->lock('')->createTTLIndex();

Once TTL index have been created (using a migration), the lock_lottery value can be set to [0, 0] in order to disable random pruning of expired locks.

Also, using a UTCDateTime is more precise than the int timestamp in seconds. So we can use updateOne as discussed here.

alcaeus

LGTM. Not sure about what prompted the change for the return value of the acquire method, but it looks like it achieves the same result.

GromNaN · 2024-04-22T11:54:15Z

LGTM. Not sure about what prompted the change for the return value of the acquire method, but it looks like it achieves the same result.

I think it saves the server from having to return the modified document, which means fewer operations. It's probably negligible, but in any case unnecessary.

alcaeus · 2024-04-22T11:57:30Z

I think it saves the server from having to return the modified document, which means fewer operations. It's probably negligible, but in any case unnecessary.

I'm not an expert, but IIRC update does have a few advantages over findAndModify, good call on that!

jmikola · 2024-04-22T14:15:03Z

src/Cache/MongoLock.php

@@ -107,6 +107,17 @@ public function forceRelease(): void
        ]);
    }

+    /** Creates a TTL index that automatically deletes expired objects. */
+    public function createTTLIndex(): void


I intentionally didn't suggest using a TTL index in #2877 because it seemed redundant with the $lottery constructor argument that remained in place.

Given that these are just public methods on the MongoLock and MongoStore classes, how do you expect users to interact with them? My understanding is that both classes would typically be used while handling a request, and this method seems more like schema setup. It makes sense for MongoDB, of course, but doesn't fit very well into whatever common API Laravel intended for these classes.

This is an additional feature that should be documented as I have described. The methods will probably be called in a migration. I think that for applications with a significant volume of traffic, using TTL indexes will have a positive impact on performance without having to search for the correct lottery value.

src/Cache/MongoLock.php

jmikola · 2024-04-22T17:44:49Z

src/Cache/MongoLock.php

-        $lockTimeout = $this->seconds > 0 ? $this->seconds : $this->defaultTimeoutInSeconds;
-
-        return $this->currentTime() + $lockTimeout;
+        return new UTCDateTime(Carbon::now()->addSeconds($additionalSeconds));


What allows UTCDateTime to be constructed from a Carbon instance? Is this relying on Carbon's string representation?

I looked at the Addition and Subtraction docs for Carbon and the strings in the examples don't look correct.

The Carbon class extends DateTime, and the UTCDateTime constructor accepts any DateTimeInterface. There is no string transformation.

Ah, I missed Carbon's inheritance. SGTM!

jmikola · 2024-04-22T17:50:12Z

src/Cache/MongoStore.php

        $result = $this->collection->findOneAndUpdate(
            [
                '_id' => $this->prefix . $key,
-                'expiration' => ['$gte' => $this->currentTime()],


By removing this criteria and the preceding call to forgetIfExpired(), you're potentially incrementing an expired document. Is that a concern?

Since the TTL index is optional, I wouldn't have expected this to change.

I've removed the previous forgetIfExpired in order to avoid useless command. A nominal usage must be a single command.

Even if the expired item is incremented here, it is deleted just after.

jmikola · 2024-04-22T17:55:45Z

src/Cache/MongoLock.php

@@ -74,11 +74,11 @@ public function acquire(): bool
            ],
        );

-        if (random_int(1, $this->lottery[1]) <= $this->lottery[0]) {
-            $this->collection->deleteMany(['expiration' => ['$lte' => $this->currentTime()]]);
+        if (! empty($this->lottery[0]) && random_int(1, $this->lottery[1]) <= $this->lottery[0]) {


Does anything enforce that $this->lottery is a tuple? This only checks that the first element exists and is non-empty, but there is no check for a second element or both being integers.

Perhaps that should be done in the constructor.

I assume you intentionally decided not to make $lottery nullable as a means to disable it.

It seemed to me that the "zero chances on whatever" value was explicit enough to say that we wanted to disable this mechanism.

jmikola · 2024-04-22T17:57:13Z

src/Cache/MongoLock.php

        }

-        return $result['owner'] === $this->owner;
+        return $result->getModifiedCount() > 0 || $result->getUpsertedCount() > 0;


Wasn't this logic problematic in the original PR? Are you now relying on the UTCDateTime always changing due to sub-second precision?

No problem with the previous logic. I said to myself that it was impossible to have the same lock with the same owner acquired in the same microsecond. But I've reverted to that, so you don't have to worry.

jmikola · 2024-04-22T17:59:42Z

src/Cache/MongoLock.php

     */
    public function __construct(
        private readonly Collection $collection,
        string $name,
        int $seconds,
        ?string $owner = null,
        private readonly array $lottery = [2, 100],
-        private readonly int $defaultTimeoutInSeconds = 86400,


Why was this removed entirely? It looks like it previously served a purpose if $seconds was zero. Now, specifying zero for $seconds would cause locks to immediately expire.

Since $defaultTimeoutInSeconds was only a default value, I moved the processing to MongoStore::lock. It is not possible to disable lock expiration.

jmikola

I trust there will be a separate PR for documentation (since this was a follow-up on an original PR).

GromNaN added this to the 4.3 milestone Apr 22, 2024

GromNaN requested review from jmikola and alcaeus April 22, 2024 09:58

GromNaN requested a review from a team as a code owner April 22, 2024 09:58

GromNaN mentioned this pull request Apr 22, 2024

PHPORM-99 Add optimized cache and lock drivers #2877

Merged

3 tasks

GromNaN force-pushed the PHPORM-99-ttl branch 2 times, most recently from 665930a to aa7bff1 Compare April 22, 2024 11:29

Enable TTL index to auto-purge of expired cache and lock items

c7fce86

GromNaN force-pushed the PHPORM-99-ttl branch from aa7bff1 to c7fce86 Compare April 22, 2024 11:30

alcaeus approved these changes Apr 22, 2024

View reviewed changes

GromNaN added 2 commits April 22, 2024 13:59

Simplify constructor arguments of MongoLock

087e8fd

Remove useless expiration condition in cache increment

e0da8e9

GromNaN force-pushed the PHPORM-99-ttl branch from b70afc9 to e0da8e9 Compare April 22, 2024 12:00

jmikola reviewed Apr 22, 2024

View reviewed changes

GromNaN added 4 commits April 23, 2024 09:43

Rename expiration field to expires_at for naming consistency

4a5a3d8

Validate lottery value

33eb655

Revert lock acquire to use findOneAndUpdate

42e890c

Fix test using UTCDateTime

8d02b31

jmikola approved these changes Apr 23, 2024

View reviewed changes

GromNaN merged commit 3de2876 into mongodb:4.3 Apr 23, 2024
23 checks passed

GromNaN mentioned this pull request Apr 24, 2024

PHPORM-174 Add doc for cache and locks #2909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

GromNaN commented Apr 22, 2024 •

edited

Loading

alcaeus left a comment

GromNaN commented Apr 22, 2024

alcaeus commented Apr 22, 2024

jmikola Apr 22, 2024

GromNaN Apr 22, 2024

jmikola Apr 22, 2024

GromNaN Apr 23, 2024

jmikola Apr 23, 2024

jmikola Apr 22, 2024

GromNaN Apr 23, 2024

jmikola Apr 22, 2024

GromNaN Apr 23, 2024

jmikola Apr 22, 2024

GromNaN Apr 23, 2024

jmikola Apr 22, 2024

GromNaN Apr 23, 2024

jmikola left a comment

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

PHPORM-99 Enable TTL index to auto-purge of expired cache and lock items #2891

Conversation

GromNaN commented Apr 22, 2024 • edited Loading

alcaeus left a comment

Choose a reason for hiding this comment

GromNaN commented Apr 22, 2024

alcaeus commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

GromNaN commented Apr 22, 2024 •

edited

Loading