vmware / splinterdb

High Performance Embedded Key-Value Store
https://splinterdb.org
Apache License 2.0
680 stars 57 forks source link

(#500) Move hook-related global vars to task_system{} struct. #528

Closed gapisback closed 1 year ago

gapisback commented 1 year ago

This commit removes the dependency of task system structures on global variables declared in task.c . The hook-related variables are now moved as members of the task_system{} struct. This removes accessing potentially stale values when task-system is destroyed and re-created. Also, TASK_MAX_HOOKS is now decreased from 8 to 4. This change largely has no functional impact, and is mostly a test-stabilization fix.


NOTE: The issue #500 describes in fuller detail why these global variables could present a problem in a testing scenario, e.g., when we do re-create a task-system by performing task_system_destroy() followed by task_system_create().

netlify[bot] commented 1 year ago

Deploy Preview for splinterdb canceled.

Name Link
Latest commit 9379b9b82afbbabb8c84ecd7f3f9fe765d6d6393
Latest deploy log https://app.netlify.com/sites/splinterdb/deploys/6410e3d86bdf7f000847d1db
rtjohnso commented 1 year ago

How about we kill the hook stuff altogether? It's used only for some IO registration thing, which we don't even use in our current platform.

gapisback commented 1 year ago

@rtjohnso - You are right that we currently "... It's used only for some IO registration thing, which we don't even use in our current platform."

But, I'd like to retain this one last remaining hook for now. I discovered that the availability of struct io_ops{}->io_thread_register_fn to be handy and useful when integrating SplinterDB with the other fast-data ingestion product integration.

It was handy for me to poke a Unix process to become a thread, and to use the IO-Ops virtual functions as a way for the process to register itself as a thread and deregister (upon disconnection). I actually extended this io_ops virtual function list to add a deregister method, too.

You don't see that code in our /main branch but having these virtual pointers and this hook-system will come of use in future integrations.

gapisback commented 1 year ago

Forgot to cc: @ajhconway -- who may have another opinion on whether we should totally nuke this hooks-business or if he would go along with my request to retain it (to facilitate future integration).

rtjohnso commented 1 year ago

If you found it useful then no problem keeping it.

Best, Rob

On Wed, Jan 11, 2023, 5:13 PM gapisback @.***> wrote:

Forgot to cc: @ajhconway https://github.com/ajhconway -- who may have another opinion on whether we should totally nuke this hooks-business it or if he would go along with my requesting to retain it (to facilitate future integration).

— Reply to this email directly, view it on GitHub https://github.com/vmware/splinterdb/pull/528#issuecomment-1379683910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA45FRN2THCIMO7JTKSUGNDWR5LBLANCNFSM6AAAAAATYDNWVQ . You are receiving this because you were mentioned.Message ID: @.***>