Page MenuHomePhabricator

[lib] Create an operation pruner
ClosedPublic

Authored by tomek on Jul 31 2024, 10:50 AM.
Tags
None
Referenced Files
F3197432: D12954.id43021.diff
Sat, Nov 9, 8:36 AM
F3197341: D12954.id43033.diff
Sat, Nov 9, 8:10 AM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Unknown Object (File)
Wed, Nov 6, 11:18 PM
Subscribers

Details

Summary

We should prune the operations periodically because their number can keep growing. Frequency and max age were chosen arbitrary, and I'm not sure if they are good.

https://linear.app/comm/issue/ENG-8768/add-a-component-that-prunes-the-updates-periodically

Depends on D12949

Test Plan

Queued some operations for two different threads with different timestamps. Modified pruner constants and verified that correct operations were removed.

Diff Detail

Repository
rCOMM Comm
Branch
dmops
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/reducers/dm-operations-queue-reducer.js
29

Shouldn't this be >=?
If I have a messages with times 700 and 900, time now is 1000 and MAX_AGE_PRUNED is 200,
then pruneMaxTime is 800, so we want to delete message with 700 timestamp, right?
And filter takes a callback that returns true for elements that are supposed to stay.
Am I misunderstanding?

I'm worried about a scenario where the user doesn't open the app for a couple of days, and when they do all ops are suddenly deleted before they can be processed. Are we protected against that somehow?

tomek planned changes to this revision.Aug 1 2024, 6:08 AM
In D12954#366380, @inka wrote:

I'm worried about a scenario where the user doesn't open the app for a couple of days, and when they do all ops are suddenly deleted before they can be processed. Are we protected against that somehow?

That's a good question. The current solution isn't terrible because for an operation to appear in this queue, it needs to be unprocessable at some point (its thread isn't present in the store). Also, pruning the messages happens at the start of the app, which means that pending messages from tunnelbroker are unlikely to be received. But regardless, I think we can do better.

Instead of relying on a timestamp from the operation, we could generate a new timestamp when putting it into the queue.

lib/reducers/dm-operations-queue-reducer.js
29

You're right, thanks!

Generate a timestamp when putting operations to the queue

lib/tunnelbroker/tunnelbroker-context.js
465 ↗(On Diff #43023)

Nit: we usually capitalize acronyms

This revision is now accepted and ready to land.Aug 2 2024, 7:13 AM
This revision was automatically updated to reflect the committed changes.