Views: cleanup unsubscribed views #3651
Conversation
Signed-off-by: Shubham Mishra <[email protected]>
Signed-off-by: Shubham Mishra <[email protected]>
Shubham8287
force-pushed
the
shub/clear-views
branch
from
204534c to
6615aac
Compare
|
Test is better modularised in this PR - #3670. |
joshua-spacetime
left a comment
joshua-spacetime
left a comment
There was a problem hiding this comment.
The approach makes sense. I just have a couple thoughts:
-
I think we should be more conservative in how frequent we clean up views. I don't think our clean up intervals need to be on the order of seconds.
-
Also we probably want to bound the length of time a cleanup job takes. If there are many old views, I think we'd rather clean them up in multiple jobs to avoid taking a tx lock for an extended period of time.
Should it be limited by a hardcoded number (like 100 cleanups at a time) Or duration (atmost to take 5ms) or something else? I feel limiting it to hardcoded number of views to cleanup can be a problem if continously more new views expires more than that number between job intervals. |
|
I agree, I think duration is the right metric. And we can even make that duration configurable later (not a requirement for this PR). |
# Description of Changes Make View backing tables and related St tables not persistent. 1. Modifies `CommittedState` to hold set of ephemeral tables. 2. Update `TxData` to contain a subset of ephemeral tables which has been modified in current transaction. `do_durability` filter those table out before writting the transaction to commitlog. depends on: #3651 # API and ABI breaking changes NA # Expected complexity level and risk 2.5. looks simple but changes comes in the hotpath, I ensured we don't do unneccessary heap allocations but patch has the potential to regress perfomance. # Testing - unit test. --------- Signed-off-by: Shubham Mishra <[email protected]> Co-authored-by: Mazdak Farrokhzad <[email protected]>
Description of Changes
A background task to cleanup unsubscribed views.
fixes #3587
API and ABI breaking changes
NA
Expected complexity level and risk
2
Testing
Added a test