valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.42k stars 656 forks source link

Support empty callback on function and free temp function in async way #1334

Closed enjoy-binbin closed 1 day ago

enjoy-binbin commented 4 days ago

We have a replicationEmptyDbCallback, it is a callback used by emptyData while flushing away old data. Previously, we did not add this callback logic for function, in case of abuse, there may be a lot of functions, and also to make the code consistent, we add the same callback logic for function.

Changes around this commit:

  1. Extend emptyData / functionsLibCtxClear to support passing callback when flushing functions.
  2. Added disklessLoad function create and discard helper function, just like disklessLoadInitTempDb and disklessLoadDiscardTempDb), we wll always flush the temp function in a async way to avoid any block.
  3. Cleanup around discardTempDb, remove the callback pointer since in async way we don't need the callback.
  4. Remove functionsLibCtxClear call in readSyncBulkPayload, because we called emptyData in the previous lines, which also empty functions.

We are doing this callback in replication is because during the flush, replica may block a while if the flush is doing in the sync way, to avoid the primary to detect the replica is timing out, replica will use this callback to notify the primary (we also do this callback when loading a RDB). And in the async way, we empty the data in the bio and there is no slw operation, so it will ignores the callback.

codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.71%. Comparing base (6038eda) to head (68ba9c7). Report is 13 commits behind head on unstable.

Files with missing lines Patch % Lines
src/replication.c 55.55% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## unstable #1334 +/- ## ============================================ - Coverage 70.71% 70.71% -0.01% ============================================ Files 115 115 Lines 63175 63178 +3 ============================================ + Hits 44675 44677 +2 - Misses 18500 18501 +1 ``` | [Files with missing lines](https://app.codecov.io/gh/valkey-io/valkey/pull/1334?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io) | Coverage Δ | | |---|---|---| | [src/db.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1334?src=pr&el=tree&filepath=src%2Fdb.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL2RiLmM=) | `89.09% <100.00%> (-0.01%)` | :arrow_down: | | [src/functions.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1334?src=pr&el=tree&filepath=src%2Ffunctions.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL2Z1bmN0aW9ucy5j) | `95.54% <100.00%> (-0.18%)` | :arrow_down: | | [src/server.h](https://app.codecov.io/gh/valkey-io/valkey/pull/1334?src=pr&el=tree&filepath=src%2Fserver.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL3NlcnZlci5o) | `100.00% <ø> (ø)` | | | [src/replication.c](https://app.codecov.io/gh/valkey-io/valkey/pull/1334?src=pr&el=tree&filepath=src%2Freplication.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io#diff-c3JjL3JlcGxpY2F0aW9uLmM=) | `87.29% <55.55%> (-0.07%)` | :arrow_down: | ... and [13 files with indirect coverage changes](https://app.codecov.io/gh/valkey-io/valkey/pull/1334/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=valkey-io)

🚨 Try these New Features:

enjoy-binbin commented 2 days ago

Why do we have this callback and what is it used for. I think it's only to notify the primary that the replica is still alive when it is doing a slow operation, right?

yes, when doing the flush in a sync way, replica will notify the primary to keep the line online.

emptyDbStructure ignores the callback if the async parameter is set. Why? Because replica doesn't need to send alive messages to the primary in this case?

yes, in the async way, we empty the data in the bio (it also don't have a way to call the callback), there is no slow operation so no need to notify the primary druing the flush.

zuiderkwast commented 1 day ago

Yes, i see you mentioned this in the pr description. looks good, thanks. Let's merge.