Merged
Conversation
JelteF
force-pushed
the
dont-always-sync-motherduck
branch
5 times, most recently
from
2fccd22 to
40b2674
Compare
Keeping a connection open to MotherDuck forever is undesirable from a resourcing perspective on the MotherDuck side. This changes that to behave similarly like the DuckDB CLI, after 5 minutes (by default) syncing will stop. The main difficulty is to restart the syncing. In the DuckDB CLI the syncing will start again after some activity is detected, but the background worker never triggers activity. So instead we need to make sure that activity in other connections triggers a restart of the syncing in the background worker. This is done using some very simple IPC in shared memory.
JelteF
force-pushed
the
dont-always-sync-motherduck
branch
from
40b2674 to
892f6c7
Compare
JelteF
commented
Feb 7, 2025
Comment on lines
+662
to
+664
| * TODO: Passing a reference through InvokeCPPFunc doesn't work here | ||
| * for some reason. So to work around that we use a pointer instead. | ||
| * We should fix the underlying problem instead. |
Collaborator
Author
There was a problem hiding this comment.
@Y-- maybe something for your template magic to fix.
Collaborator
There was a problem hiding this comment.
I think I've seen something like that in the past. Never got a chance to investigate though... Let's open an issue and I'll try to look at it soon?
Y--
approved these changes
Feb 11, 2025
Collaborator
Y--
left a comment
Y--
left a comment
There was a problem hiding this comment.
LGTM, main comment is around timeout default
Co-authored-by: Y. <y.lemaout@gmail.com>
This reverts commit 488e2cf.
Y--
approved these changes
Feb 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keeping a connection open to MotherDuck forever is undesirable from a resourcing perspective. This changes that to behave similarly to the DuckDB CLI: After 5 minutes (by default) syncing will stop. The main difficulty is to restart the syncing. In the DuckDB CLI the syncing will start again after some activity is detected, but the background worker never triggers activity. So instead we need to make sure that activity in other connections triggers a restart of the syncing in the background worker. This is done using some very simple IPC in shared memory.