Remove unused dict callback parameter from functionReset and related functions#3357
Conversation
…functions The callback parameter in functionReset(), functionsLibCtxFree(), and functionsLibCtxClear() was never actually used - it was always passed as NULL or cast from an incompatible type. This cleanup: - Removes the void(callback)(dict *) parameter from functionReset() - Removes the callback parameter from functionsLibCtxFree() - Removes the callback parameter from functionsLibCtxClear() - Makes functionsLibCtxReleaseCurrent() static since it's only used internally - Removes the TODO comment in db.c about the callback incompatibility - Updates all call sites to use the simplified function signatures The dictEmpty() calls now explicitly pass NULL for the callback, which was effectively what was happening before since the parameter was unused. Signed-off-by: Justin Fung <justfung@amazon.com>
There was a problem hiding this comment.
Pull request overview
This PR simplifies the functions library reset/free APIs by removing an unused dictEmpty() progress-callback parameter from functionReset() and related functions, and updates all call sites accordingly.
Changes:
- Removed the
void(callback)(dict *)parameter fromfunctionReset(),functionsLibCtxFree(), andfunctionsLibCtxClear(). - Made
functionsLibCtxReleaseCurrent()staticsince it is only used withinsrc/functions.c. - Updated call sites in
db.candlazyfree.cto match the new signatures and removed the old callback cast/TODO.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lazyfree.c | Updates functionsLibCtxFree() call sites to the new signature. |
| src/functions.h | Updates public declarations for the simplified functions reset/free APIs. |
| src/functions.c | Removes callback plumbing, passes NULL to dictEmpty(), and makes functionsLibCtxReleaseCurrent() static. |
| src/db.c | Simplifies emptyData()’s function reset path and removes the prior callback cast/TODO. |
You can also share your feedback on Copilot code review. Take the survey.
| if (with_functions) { | ||
| serverAssert(dbnum == -1); | ||
| /* TODO: fix this callback incompatibility. The arg is not used. */ | ||
| functionReset(async, (void (*)(dict *))callback); | ||
| functionReset(async); | ||
| } |
There was a problem hiding this comment.
emptyData() previously forwarded its progress callback into functionReset() (via cast) so dictEmpty() could periodically invoke it during function-library cleanup (notably replicationEmptyDbCallback, which keeps the primary from timing out during full resync). With the new functionReset(async) signature, that callback is no longer invoked while flushing functions, which can block long enough to cause replication timeouts on large function libraries. Consider preserving progress-callback behavior here (e.g., provide a functionResetWithProgressCb internal helper, or adapt the existing void(callback)(hashtable *) into a dict *-typed callback that ignores the argument and calls the progress hook).
| void functionsLibCtxClear(functionsLibCtx *lib_ctx) { | ||
| dictEmpty(lib_ctx->functions, NULL); | ||
| dictEmpty(lib_ctx->libraries, NULL); |
There was a problem hiding this comment.
functionsLibCtxClear() now calls dictEmpty(..., NULL) unconditionally. The dictEmpty() callback is designed to be invoked periodically during large clears (see dictClear() calling it every 65536 buckets) and was previously used as a progress hook from emptyData() during synchronous flushes (e.g., replication sends periodic newlines). With this change, synchronous function-library flushes may no longer yield/provide progress signaling, which can introduce long blocking periods and replication timeouts. Suggest reintroducing an internal progress callback path (even if the public API stays simplified) so callers like emptyData() can continue to pass a progress hook.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3357 +/- ##
============================================
+ Coverage 74.47% 74.51% +0.04%
============================================
Files 130 130
Lines 72719 72719
============================================
+ Hits 54155 54185 +30
+ Misses 18564 18534 -30
🚀 New features to boost your workflow:
|
rainsupreme
left a comment
rainsupreme
left a comment
There was a problem hiding this comment.
Looks good for the most part - I don't think this will introduce a regression on its own looks like, but the AI raises an interesting point about tracking progress for large number of functions and avoiding latency spikes. No idea if that's a real concern but I'd like to learn a bit more. (Possibly as a separate issue from this PR, since it would've been introduced earlier.)
Also, worth noting that dict will soon be just a hashtable wrapper: #3366
Summary
This PR removes the unused
void(callback)(dict *)parameter fromfunctionReset()and related functions.Changes
The callback parameter in
functionReset(),functionsLibCtxFree(), andfunctionsLibCtxClear()was never actually used - it was always passed as NULL or cast from an incompatible type. This cleanup:void(callback)(dict *)parameter fromfunctionReset()functionsLibCtxFree()functionsLibCtxClear()functionsLibCtxReleaseCurrent()static since it's only used internallyFiles Changed
src/functions.h: Updated function declarationssrc/functions.c: Removed callback parameters and updated implementationssrc/db.c: SimplifiedfunctionReset()call, removed TODO commentsrc/lazyfree.c: UpdatedfunctionsLibCtxFree()callsRationale
The
dictEmpty()calls now explicitly pass NULL for the callback, which was effectively what was happening before since the parameter was unused. This simplifies the API and removes the need for the type cast workaround that was previously used in db.c.