Monitor changes to tool/list revisions without using watchers
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	Slst2020
	Feb 16 2022, 12:58 PM

Description

ToolView.vue and ListView.vue both use watchers to monitor changes in the "patrolled" status of a revision.

In practice, this results in ~180 API calls and ~2-3 MB of data per minute. (in production; on the dev server it's >1800 calls/min, and it makes my fans go into overdrive), just to monitor if the revision has been marked as patrolled from another part of the application. This happens regardless of whether the revision is marked as patrolled to begin with.

If you switch to another browser tab, this network traffic keeps running in the background. While maybe not a "big deal", this could potentially be problematic for users in countries where mobile data is expensive.

My suggestion would be to remove the watchers until we have found a better solution to make these views reactive to patrolling status changes.

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	toolhub: Bump container version to 2022-03-15-002555-production	operations/deployment-charts	master	+1 -1
	bug: patrol tool/list revisions without using watchers	wikimedia/toolhub	main	+118 -25

Customize query in gerrit

Event Timeline

Slst2020 created this task.Feb 16 2022, 12:58 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 16 2022, 12:58 PM

I have seen this myself locally, and I think it is a bug. I can't quite figure out what edge case Srishti was trying to work around when she first wrote that watch into the code. I minor testing in my dev tree I couldn't see anything breaking when I just commented out the entire watch in ToolView.vue.

In T301870#7715129, @bd808 wrote:

I have seen this myself locally, and I think it is a bug. I can't quite figure out what edge case Srishti was trying to work around when she first wrote that watch into the code. I minor testing in my dev tree I couldn't see anything breaking when I just commented out the entire watch in ToolView.vue.

The watcher was introduced when the patrolling actions were added. If you open two browser tabs, one with a Tool's History Page and another with a specific, unpatrolled revision of the Tool, then marking the Tool as patrolled in the History Page also marks it as patrolled in the Revisions Page reactively.

As far as I'm able to tell, removing the watcher only affects the behavior described above and nothing else.

In T301870#7715141, @Slst2020 wrote:

As far as I'm able to tell, removing the watcher only affects the behavior described above and nothing else.

Ok, this makes some sense. Real-time notification of every change to every client is not really scalable, so although I can see some desire for this behavior I don't think it can be sustained.

I think there may be behavior in the API that this is intended to work around. The /patrol/ endpoints for both tools and lists will return a 404 status when called on a revision that is already marked with patrolled=True in the database. The markRevisionAsPatrolled methods in both the tools and lists vuex modules will currently raise this as an error. I think maybe we should update that on the API side so that there is a unique error code (there are other ways to trigger a 404 from those endpoints) and then add frontend handling to treat a "you tried to mark this as patrolled, but it was already marked as patrolled by someone else" response from the API as successful.

bd808 changed the subtype of this task from "Task" to "Bug Report".Feb 16 2022, 5:18 PM

In T301870#7715407, @bd808 wrote:

I think maybe we should update that on the API side so that there is a unique error code (there are other ways to trigger a 404 from those endpoints) and then add frontend handling to treat a "you tried to mark this as patrolled, but it was already marked as patrolled by someone else" response from the API as successful.

+1 for this solution

bd808 triaged this task as High priority.Feb 25 2022, 11:41 PM

bd808 moved this task from Backlog to Groomed/Ready on the Toolhub board.

bd808 changed the task status from Open to In Progress.Feb 28 2022, 2:52 AM

bd808 claimed this task.

bd808 moved this task from Groomed/Ready to In Progress on the Toolhub board.

bd808 added a project: Developer-Advocacy (Jan-Mar 2022).

Restricted Application added a project: User-bd808. · View Herald TranscriptFeb 28 2022, 2:52 AM

Change 766319 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[wikimedia/toolhub@main] bug: patrol tool/list revisions without using watchers

https://gerrit.wikimedia.org/r/766319

gerritbot added a project: Patch-For-Review.Feb 28 2022, 3:43 AM

Change 766319 merged by jenkins-bot:

[wikimedia/toolhub@main] bug: patrol tool/list revisions without using watchers

https://gerrit.wikimedia.org/r/766319

Maintenance_bot removed a project: Patch-For-Review.Feb 28 2022, 2:11 PM

bd808 closed this task as Resolved.Feb 28 2022, 4:33 PM

Change 770638 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[operations/deployment-charts@master] toolhub: Bump container version to 2022-03-15-002555-production

https://gerrit.wikimedia.org/r/770638

gerritbot added a project: Patch-For-Review.Mar 15 2022, 12:35 AM

Change 770638 merged by jenkins-bot: