The EchoNotificationJob contains non-scalar value in it's parameters, the event property is an instance of EchoEvent, thus it's serialization relies on the redis-base JobQueue ability to use PHP serialization.
The kafka-based queue deliberately doesn't support PHP serialization and requires all the jobs to be JSON-serializable per T161647, so the EchoNotificationJob job should be changed and made JSON-serializable.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • Pchelolo | T157088 [EPIC] Develop a JobQueue backend based on EventBus | |||
| Resolved | • Pchelolo | T190327 FY17/18 Q4 Program 8 Services Goal: Complete the JobQueue transition to EventBus | |||
| Resolved | SBisson | T192945 Make EchoNotification job JSON-serializable | |||
| Resolved | PRODUCTION ERROR | SBisson | T194848 Fatal error: $this is null in Echo/includes/model/Event.php on line 345 |
Change 431831 had a related patch set uploaded (by Sbisson; owner: Sbisson):
[mediawiki/extensions/Echo@master] Make NotificationJob json-serializable
Change 431831 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Make NotificationJob json-serializable
Change 432574 had a related patch set uploaded (by Sbisson; owner: Sbisson):
[mediawiki/extensions/Echo@master] Remove NotificationJob backward compatibility
Let's keep it open until this hits production. @SBisson could you SWAT the change to speed up the process?
This is broken in production, see T194848: Fatal error: $this is null in Echo/includes/model/Event.php on line 345
Change 433441 had a related patch set uploaded (by 20after4; owner: 20after4):
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.4] Revert "Make NotificationJob json-serializable"
Change 433441 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.4] Revert "Make NotificationJob json-serializable"
Change 433670 had a related patch set uploaded (by Sbisson; owner: Sbisson):
[mediawiki/extensions/Echo@master] Use static newFromID instead of loadFromID
Change 433670 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use static newFromID instead of loadFromID
This still didn't quite help. Although the event property is not there anymore, the job is still not serializable, because masterPos property contains an instance of Wikimedia\Rdbms\MySQLMasterPos class.
I've raised the priority to HIGH since this is the only job left on the old queue and it's blocking a bunch of work related to decommissioning Redis instances, stopping the jobrunner services etc.
After a while of digging through the Echo code, here's what I've found: the notification job includes an instance of DBMasterPos for both main and echo DB, and then before execution, it waits for replication to the master position at the time of submitting the job. This is done to avoid cases where the JobQueue executed the event so quickly that replicas still couldn't catch up, so the job would fail.
The only implementation of the DBMasterPos we have right now in core, MySQLDBMasterPos is quite easily serializable into JSON, but we obviously can not assume that the DBMasterPos is MySQL at the time of deserialization, so making it properly serialize/deserialize to/from JSON requires quite a lot of changes to really high-level MediaWiki interface, for example IDatabase, which is not a great idea for such a small use-case.
Alternatively, we probably could wait for master position at the time of executing the event, but that's not quite nice since we would need to open a possibly unnecessary connection to the master, and might wait a bit longer.
Another possibility is to remove this logic altogether and rely on job queue retry semantics if a particular job fails because of the replication lag. Although, that would add some delays in case the failure happens.
Any ideas what the best course of action would be here? I lack product knowledge for Echo, how bad would a slight delay in notification delivery be?
I think you've described accurately what's happening and why.
Echo events can be delivered immediately or using the job queue (this is configurable per event type). Adding a slight delay in the latter case shouldn't cause any problem since there is no expectation that they will arrive quickly to start with.
Alternatively, we probably could wait for master position at the time of executing the event, but that's not quite nice since we would need to open a possibly unnecessary connection to the master, and might wait a bit longer.
I think this is the less bad option. I'd be willing to try it and see if anything goes wrong in the DB metrics (I assume we have those). It's fairly easy to try for a few days and re-evaluate but I'll let @Catrope make the call.
Change 440894 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Echo@master] Remove masterPos from the job specification.
Change 441818 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.8] Remove masterPos from the job specification.
Change 440894 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Remove masterPos from the job specification.
Change 441818 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.8] Remove masterPos from the job specification.
Mentioned in SAL (#wikimedia-operations) [2018-06-25T13:34:41Z] <hashar@deploy1001> Synchronized php-1.32.0-wmf.8/extensions/Echo: Remove masterPos from the job specification - T192945 (duration: 00m 58s)
Change 441868 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.999] Remove masterPos from the job specification.
Change 441868 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.32.0-wmf.999] Remove masterPos from the job specification.
Mentioned in SAL (#wikimedia-operations) [2018-06-25T14:17:13Z] <hashar@deploy1001> Synchronized php-1.32.0-wmf.999/extensions/Echo: Remove masterPos from the job specification - T192945 (duration: 00m 59s)