[22:42:06] AaronSchulz: There's a comment here about avoiding using-facing db errors over code that uses a pre-send update. That seems contradictory, right? https://github.com/wikimedia/mediawiki/blob/3c7af0fd5c8d57928885a715ad866b6879a1e6c6/includes/page/WikiPage.php#L1320-L1332 [22:54:11] Krinkle: it seems odd. Only GUI errors are suppressed, though at least the main trx and other presend updates would still run. I think maybe deferredupates behavior was different in the past. [23:11:49] Krinkle: either the try/catch should go back or the comment could be removed [23:13:30] AaronSchulz: or we make it post-send? [23:14:05] If it's not going to change the output, there's no point in waiting an additional few milliseconds for it [23:15:15] and in terms of user feedback, it's going to finish before the user received the HTML either way, so long as the laws of physics make it impossible for anyone to receive, parse and render anything from the network in under 10ms. [23:18:16] Krinkle: I guess it depends whether a replica is lagged or not (since CP won't apply for post-send stuff) [23:26:06] AaronSchulz: hm.. Interesting. [23:26:23] Why is that different for pre-send? [23:26:32] Just because it's in the main trx? [23:26:37] I thought that's after the trx thouguh [23:32:27] Krinkle: well, you don't know how many deferredupdates will run, shutdown() is down after all of them via << $lbFactory->shutdown( $lbFactory::SHUTDOWN_NO_CHRONPROT ); >> in restInPeace(). [23:34:27] I guess it could try some kind of "more optimistic" 2nd shutdown to update the CP pos store. Though it's too late to set cookies or change the url at that point. If the update is postsend, the positions are not known until it's too late though. [23:35:01] there could be a simpler cookie to just "try to avoid replicas with > X ms lag" or something. [23:35:55] such a thing would have to be triggered be some explicit call from the code that added the post-send update though....hrm [23:37:18] AaronSchulz: I think I misunderstood what you said. I interpreted your comment to mean that pre-send updates might not see the result of the main trx. I believe what you meant was that the CP cookie for the next page view will not take post-view updates into consideration. That makes sense indeed. [23:37:23] maybe there view updates could have a presend step that checks if a write seems needed and schedules that post send (along with setting some flag to try harder to avoid lagged replicas) [23:37:24] post-send updates* [23:39:18] a simpler idea would be to just use a simpler store that is less prone to lag due to unpredictable complex updates [23:40:05] It does two things: onPageViewUpdate (unused in prod), and clearTitleUserNotifications. [23:40:12] clearTitleUserNotifications queues a post-send update for its db write. [23:40:32] (and that has relaxed use of fsync...could even be mainstashdb really...which is partly is afaik) [23:41:01] and for the resetNotificationTimestamp call it uses the main stash with a post-send job queue. [23:41:16] so seems like none of that affects pre-send, including the CP cookie [23:41:32] doing so would ofc have been flagged as a DBPerformance violation if it did make a write [23:47:27] looks like it already is two-stage then, the comment is just outdated [23:59:06] k