So just hard deprecate the entire method?
Sounds good. Is it possible to use UserIdentity instead of the User?
See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/596502 - we opted for just removing the use of this method entirely. Should probably do the same in other occurrences if they exist
Actually, after a bit of more thinking, I wonder if LogEntry should instead be converted into a value object - only holding the fields with getters/setters and containing no logic, and database operations, and all the stuff that uses the dependency should be done via a separate service, or even a group of services.
Will do; is this then approved from a product point of view?
All patches here were deployed. I've done some logstash searching, and all errors have completely stopped around May 04-05 which corresponds with deployment. Resolving. Please reopen if reappears.
Verified that the error has gone away. Resolving.
@ssastry please correct me if I'm wrong, but Linter extension stores Parsoid lints in MySQL. If I understand it well, the flow is the following: page edit -> parsoid rerender -> parsoid calling MW API to record lint errors -> API is scheduling a job -> errors are stored in the database. So, VE doesn't need to call RESTBase to re-render the lint errors, it can just execute an action API query if working from the client side, or depend on Linter extension and get the errors from linter table directly.
ManualLogEntry implements a LogEntry interface, and has a sibling: DatabaseLogEntry with RCDatabaseLogEntry as a specialized dependent. DatabaseLogEntry is only created via static factory methods, not via direct construction.
Wed, May 27
We'd still need to figure out how to make eventgate-wikimedia generate static stream config from EventStreamConfig API at runtime. We'd have to have some script that runs during deployment and gets all stream configs that match stream_config_allowed_settings and renders out a static stream-config.yaml file that is used as the stream_config_uri.
Tue, May 26
The two patches above introduce the new hook parameter and demonstrate it's usage in an extension. I will make patches to the rest of the extensions once the review of the core patch is completed and we're in agreement on the API.
Fri, May 15
Some tests for the new service would be nice.
The patches were merged, but have not been deployed yet. I'm on vacation next week, so to unblock you I'm asking Hugh to deploy restbase with your changes next week
Thu, May 14
I very much support this proposal.
Wed, May 13
I was asking @Pchelolo as the person who filed the task, but sure. :-)
Tue, May 12
I'm not sure what else to do here.
It seems to me like the only thing useful for the purge handler is the timestamp of when the purge event was created by the producer. Anything about what Last-Modified will be or when some cascading chain started seems like things it either can't or shouldn't use, right? Having said that - what will this actually be used for?
Given that the end goal is not just decoupling, but also moving us closer to fixing the problems with the table described by @Krinkle in T146585#4233276 we need to change the API to perhaps eventually disallow NULL user_last_timestamp so that we're able to switch to upsert and remove duplicated rows, so I have deprecated passing null revision into setNewtalk. However, maybe this requires course correction.
Both user_id and user_ip in Postgres now have default values, so this should be fixed. See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/589345
We got a few more:
Is his done? Can this be closed?
Mon, May 11
We've actually broken purging again, bu fixed now. We are going to finish working on purges soon, so we would stop breaking this.
Fri, May 8
You're talking about mobile-html endpoint right? It supports providing a revision ID to require a new revision. On normal views it's better to use URI without a revision ID, so that you have better cache hit ratio, but after en edit supplying revision ID in the request will have a lot of benefits:
Now that the TalkPageNotificationManager was introduced, we can start using it. However, here it will not be a one-to-one replacement for calling methods.
So this means that beta will be up to date as soon as the updater patch merges. We hit a small snag there, but I think this will happen later today. @Reedy you can speed things up be reviewing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/595139 :)
Tested again. The content is ok after edits.
Thu, May 7
That makes more sense. This one's to blame: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/589699
Hey @thcipriani yeah... somehow the issue stacked up on this train.
Please give it about 10-20 minutes before verifying, it needs to catch up on the backlog
I've had a similar idea before in T237692 but I like this proposal better.
I'm closing this as a duplicate of T252145 - that approach would be better since not all the representations exist as core MW concepts.
@JoeWalsh should be fixed now. Our sincere apologies, we are going to be done messing with this soon.
I know what's going on. That's me and @hnowlan breaking things. Will fix right away.
Wed, May 6
My bad, missed it in the review :(
@Pchelolo thanks for updating this documentation - are there specifics about which characters are allowed and which characters need to be encoded?
Tue, May 5
Apologies for the delays. It has happened this morning.
The FileRepresentation spec calls for 'size' property. However, sine we're doing the thumbnailg lazily it is impossible to know at the time of returning the response. Switching to non-lazy generation of thumbnails just to know the size is not acceptable from the performance perspective. We will be dropping the 'size' key from the response.
cc @Ottomata ^^ it seems like all these are simple enough to get into our api-request schema.
Mon, May 4
But I guess you'll need to do a new chart release and deploy that in order for the new config to take effect — is that correct?
RESTBase configuration deployed.
RESTBase was enabled on the new domain: https://vi.wikipedia.beta.wmflabs.org/api/rest_v1/
Sat, May 2
I don't think this is much of an improvement - with all the trouble of updating all the extension, we could probably do a more proper solution, like ArticleFactory interface with some MW service implementing it. However, I am not sure we're ready to be doing something like this, and in general untrue of the future of the Article class.
All the instances of deprecated calls mentioned in this task have been cleaned up, except AbuseFilter. There it seems like the code is correct, but we've been using a cached serialized variable - thus calling on Article. Let's see if it resurrects next week.
Fri, May 1
So, one thing that's touching the code around it is: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/590471 - not very related, just fyi.
Ok, heh.. the fix wasn't actually deployed it seems. It's been merged, but not yet moved into production. I'll ping you when it is.
Thu, Apr 30
Complete stack trace:
It's PageEditStash. See my comment above.
The other associated log actually gives more insight on which lock is making the transaction timeout:
@daniel I don't think this is that lock showing up here. Reading the code, it seems like if that lock was taken, we should've seen a bunch more queries in this transaction, like some deletes, read from the archive table etc.
The content that was already stored will not be regenerated right away after this change. If you want to regenerate all the content, I guess we'd have to truncate mobile-html table, or do a dump or bump the mobile-html version
Looking at the requestId of one of these errors, the following log messages are found: https://logstash.wikimedia.org/goto/3e387a16e01ae0f02e72b413d6691539
Seems like all the mysteries here have been resolved. Additinally, I verified we're purging mobile-html in the same cases when we purge the old mobile-sections, so we should be good.