PHPORM-81 implement `mongodb` driver for batch #2904

GromNaN · 2024-04-23T20:07:59Z

Checklist

Add tests and ensure they pass
Add an entry to the CHANGELOG.md file
Update documentation for new features

GromNaN · 2024-05-02T17:55:12Z

tests/Bus/MongoBatchRepositoryTest.php

+                $_SERVER['__progress.count']++;
+            })
+            ->then(function (Batch $batch) {
+                $_SERVER['__then.batch'] = $batch;


We can't use $this because the closure is serialized.

How would $this be used here even if it was supported?

Offhand, is laravel/serializable-closure used internally for serialization? AFAIK, the stock serialize() function doesn't support closures on its own.

All the $_SERVER['something'] could have been replaced by $this->data['something']. But that's not allowed by the package you mentioned.

alcaeus

I'm not sure about assuming that any string passed will be a valid ObjectId. A possible workaround would be to try and create an ObjectId instance, but leave the value as-is if it isn't valid. That way, the operation will either succeed (if such a record exists) or be a no-op (which would be the assumed behaviour for any batch that doesn't exist). As is, passing a string that isn't ObjectId-like would result in an exception, which IMO is not desirable.

alcaeus · 2024-05-06T07:32:15Z

src/Bus/MongoBatchRepository.php

+    public function get($limit = 50, $before = null): array
+    {
+        if (is_string($before)) {
+            $before = new ObjectId($before);


Beware that this may throw an exception if the string is not a well-formed ObjectId. Not sure if that's something we want to consider, but I figured I'd point it out.

In the store method, the _id is not set, so only ObjectId can be inserted.

I wanted to point this out as theoretically users could call the method with any string. No objection to assuming valid ObjectIDs are passed and leaving this as-is.

src/Bus/MongoBatchRepository.php

alcaeus · 2024-05-06T07:34:55Z

src/MongoDBQueueServiceProvider.php

+        if (! isset($config['collection']) && isset($config['table'])) {
+            trigger_error('Since mongodb/laravel-mongodb 4.4: Using "table" option for the queue is deprecated. Use "collection" instead.', E_USER_DEPRECATED);
+            $config['collection'] = $config['table'];
+        }


jmikola

Forgive me for any ignorant questions.

docs/queues.txt

jmikola · 2024-05-06T17:14:50Z

src/Bus/MongoBatchRepository.php

+
+// Extending DatabaseBatchRepository is necessary so methods pruneUnfinished and pruneCancelled
+// are called by PruneBatchesCommand
+class MongoBatchRepository extends DatabaseBatchRepository implements PrunableBatchRepository


Is there any reason for inconsistent use of "Mongo" and "MongoDB" as class prefixes? This isn't the first time I've noticed it, and I understand that there may be historical inconsistencies in the project, but this PR introduces two new classes with inconsistent names.

I also named MongoLock and MongoStore with the same unsaid naming rule, following the naming examples of MongoConnector, MongoQueue ...

src/Bus/MongoBatchRepository.php

src/MongoDBQueueServiceProvider.php

jmikola · 2024-05-06T17:37:08Z

tests/Bus/MongoBatchRepositoryTest.php

+                $_SERVER['__progress.count']++;
+            })
+            ->then(function (Batch $batch) {
+                $_SERVER['__then.batch'] = $batch;


How would $this be used here even if it was supported?

Offhand, is laravel/serializable-closure used internally for serialization? AFAIK, the stock serialize() function doesn't support closures on its own.

CHANGELOG.md

src/Bus/MongoBatchRepository.php

https://github.com/laravel/framework/releases/tag/v10.37.2

jmikola

You can decide which of my comments are worth addressing.

docs/queues.txt

src/Bus/MongoBatchRepository.php

src/MongoDBBusServiceProvider.php

jordan-smith721

Just a few suggestions for the docs portion

docs/queues.txt

Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>

rustagir

approving with comments, when this PR is merged I will make an edits PR!

rustagir · 2024-05-21T14:04:38Z

docs/queues.txt

@@ -38,7 +38,7 @@ the driver in ``config/queue.php``:
     - **Required**. Specifies the queue driver to use. Must be ``mongodb``.

   * - ``connection``
-     - Uses the default connection by default. The database connection used to store jobs. It must be a ``mongodb`` connection.
+     - The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.


Suggested change

- The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

- Specifies the database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

rustagir · 2024-05-21T14:06:22Z

docs/queues.txt

           'queue' => 'default',
-           'expire' => 60,
+           'retry_after' => 60,
       ],
   ],



S: add an introductory sentence before the table, such as:
"The following table describes settings that you can specify to configure the behavior of the queue:"

rustagir · 2024-05-21T14:07:16Z

docs/queues.txt

+     - The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.
+
+   * - ``collection``
+     - **Required**. Name of the MongoDB collection to store jobs to process.


Suggested change

- **Required**. Name of the MongoDB collection to store jobs to process.

- **Required**. Specifies the name of the MongoDB collection to store jobs to process.

Applies to all entries

rustagir · 2024-05-21T14:07:58Z

docs/queues.txt

-       'database' => 'mongodb-job',
-       'table' => 'failed_jobs',
+       'database' => 'mongodb',
+       'collection' => 'failed_jobs',
   ],



S: same as earlier, add an introductory sentence to introduce this table

GromNaN changed the title ~~PHPORM-81 implement mmongodb driver for batch~~ PHPORM-81 implement mongodb driver for batch Apr 25, 2024

GromNaN force-pushed the PHPORM-81 branch 2 times, most recently from 82ff224 to 7264181 Compare May 2, 2024 15:12

GromNaN marked this pull request as ready for review May 2, 2024 17:53

GromNaN requested review from a team as code owners May 2, 2024 17:53

GromNaN requested a review from jmikola May 2, 2024 17:53

GromNaN commented May 2, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-81 branch from 0a4746d to bd20e72 Compare May 2, 2024 17:56

rustagir self-requested a review May 2, 2024 17:58

GromNaN force-pushed the PHPORM-81 branch from bd20e72 to 9d8e48d Compare May 2, 2024 18:02

GromNaN requested a review from alcaeus May 3, 2024 12:29

GromNaN force-pushed the PHPORM-81 branch from 0ea06f0 to 73527f2 Compare May 3, 2024 12:33

GromNaN changed the base branch from 4.3 to 4.4 May 3, 2024 13:05

alcaeus reviewed May 6, 2024

View reviewed changes

jmikola reviewed May 6, 2024

View reviewed changes

jmikola reviewed May 7, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

jmikola reviewed May 7, 2024

View reviewed changes

src/Bus/MongoBatchRepository.php Outdated Show resolved Hide resolved

GromNaN force-pushed the PHPORM-81 branch from 8903a48 to fc7d8ec Compare May 20, 2024 13:09

GromNaN added 11 commits May 20, 2024 15:34

PHPORM-81 implement mmongodb driver for batch

9933b78

wip

6df5604

First test

408cf08

Import Laravel tests

556cd39

-

f882b29

Update docs and review option names

a711c4d

Adjust constraint for PendingBatch::progress

4a30370

https://github.com/laravel/framework/releases/tag/v10.37.2

Update changelog

d2b4942

Doc fix

8dbdbd3

Add getUTCDateTime using Carbon::now

c542ed8

Wrapping callback not necessary

212a8cc

Update phpstan baseline

c02c7ca

GromNaN force-pushed the PHPORM-81 branch from fc7d8ec to c02c7ca Compare May 20, 2024 14:14

jmikola approved these changes May 20, 2024

View reviewed changes

docs/queues.txt Outdated Show resolved Hide resolved

src/Bus/MongoBatchRepository.php Show resolved Hide resolved

src/Bus/MongoBatchRepository.php Outdated Show resolved Hide resolved

src/MongoDBBusServiceProvider.php Show resolved Hide resolved

Skip find query when storing a batch

b912ce7

GromNaN enabled auto-merge (squash) May 20, 2024 14:46

GromNaN disabled auto-merge May 20, 2024 14:47

Update docs

5cfed4b

GromNaN requested a review from jmikola May 20, 2024 14:54

jmikola approved these changes May 20, 2024

View reviewed changes

GromNaN enabled auto-merge (squash) May 20, 2024 15:27

jordan-smith721 requested changes May 20, 2024

View reviewed changes

Apply suggestions from code review

d0fd299

Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>

GromNaN requested a review from jordan-smith721 May 21, 2024 07:09

rustagir approved these changes May 21, 2024

View reviewed changes

GromNaN removed the request for review from jordan-smith721 May 21, 2024 14:57

jordan-smith721 approved these changes May 21, 2024

View reviewed changes

GromNaN merged commit b040bef into mongodb:4.4 May 21, 2024
24 checks passed

GromNaN deleted the PHPORM-81 branch May 21, 2024 15:21

rustagir mentioned this pull request Jun 4, 2024

DOCSP-39849: revise job batching docs #2994

Merged

3 tasks

masterbater mentioned this pull request Jul 22, 2024

add driver mongodb job batching #2551

Closed

	- The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.
	- Specifies the database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

	- Required. Name of the MongoDB collection to store jobs to process.
	- Required. Specifies the name of the MongoDB collection to store jobs to process.

PHPORM-81 implement mongodb driver for batch #2904

PHPORM-81 implement mongodb driver for batch #2904

Uh oh!

Conversation

GromNaN commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jordan-smith721 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustagir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PHPORM-81 implement `mongodb` driver for batch #2904

PHPORM-81 implement `mongodb` driver for batch #2904

GromNaN commented Apr 23, 2024 •

edited

Loading

alcaeus May 6, 2024 •

edited

Loading