FEATURE: Create revision when bulk moving topics (PR #10802)

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/no-notes-in-revisions-when-bulk-moving-topics/116152/4

Is there a big performance hit here? What is the time diff between this and original when moving 30 topics?

I compared moving 30 topics in my local dev environment. Here are the results:

Old behavior (3114.1 ms)

duration (ms) from start (ms) query time (ms)
PUT http://localhost:3000/topics/bulk 33.0 +0.0
Executing action: bulk 3081.1 +32.0

New behavior (5454.9 ms)

duration (ms) from start (ms) query time (ms)
PUT http://localhost:3000/topics/bulk 46.6 +0.0
Executing action: bulk 5408.2 +46.0

So, it’s roughly a 75% increase.

@SamSaffron what do you think of it having seen @gschlager 's reply?

I think this is too slow, we are going to hit timeouts due to this change.

We have already had numerous complaints about slow moves.

@gschlager could we perhaps move the extra work to a background job? I don’t think the revisions need to appear instantly.

If we move the creation of the revision into a job we would need to force PostReviser to create a revision even though the category has already changed. Probably not the best solution, but it could work.

Or we could move all bulk operations into a hijack block. That should give us 90 seconds instead of 30 if I remember the timeouts correctly. All bulk operations would profit from such a change. What do you think?

hijack is not limited to 90 seconds, but it can clog our background queue for a single unicorn if you keep something going for a real long time. We can certainly improve this if we lean on this more.

One change that can certainly help here is to move the posts is small batches, 10 at a time, then we could provide feedback to end users after each chunk. That seems like the simplest non radical change.

hijack is not limited to 90 seconds

@SamSaffron I was under the impression it is, because hijack uses Scheduler::Defer.later which has a default timeout of 90 seconds.

Anyway, executing it in batches sounds like a good idea. I’m closing this PR for now and will reopen when I have an update.

1 Like

Oh oops, sometimes I forget about code I wrote

That sounds right then, we can certainly improve this with custom timeouts and extra threads, but it all gets terribly messy

1 Like

I updated the PR and added the new create_revision_on_bulk_topic_moves site setting (enabled by default). cc @jomaxro

I think in conjunction with https://github.com/discourse/discourse/pull/10980 this should be safe to merge.