Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗️ Persist the session #372

Merged
merged 5 commits into from
Nov 23, 2022
Merged

🏗️ Persist the session #372

merged 5 commits into from
Nov 23, 2022

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Nov 23, 2022

@falexwolf falexwolf changed the title 🏗️ Persist the session 🏗️ Persist the session Nov 23, 2022
@Koncopd
Copy link
Member

Koncopd commented Nov 23, 2022

Every call of settings.instance.db_engine() synchronizes the local sqlite file with cloud. If the session is stored, then this synchronization doesn't happen anymore.

@falexwolf
Copy link
Member Author

Ahhh, thanks for the hint! I moved this logic over to lndb-setup and I'll think what to do about this!

@falexwolf
Copy link
Member Author

My current thought is that the main thing this means is that we need to mutual exclusion check for the sqlite file. And we might need more control for influencing synching for the user.

@Koncopd
Copy link
Member

Koncopd commented Nov 23, 2022

The problem with storing the session is that the db file is synchronized only once on session initialization. Actually, this change might break a lot of things. For example, if the cached db file is deleted, it won't be synchronized anymore as far as i understand.

Copy link
Member Author

falexwolf commented Nov 23, 2022

Yes, I see. It’s an experimental change that’s deeply grounded in how users should experience lazily loading sqlalchemy objects.

Copy link
Member Author

I’ll walk you through it. And if there is another solution, then that’d be great.

Copy link
Member Author

To note: In the current implementation, the old behavior (function-scoped sessions) is just an environment variable away. So, we can just switch it back anytime.

@Koncopd
Copy link
Member

Koncopd commented Nov 23, 2022

What happens with the stored session when the cached db file is deleted? Or replaced? Maybe it is better to initiate sync of the db file on every session() call. If it doesn't break the session, then it is prob the solution.

Copy link
Member Author

We can do that, with some caveats; let’s try scoping both the session and the cache check with the InstanceSession lifetime (which is essentially until call of lndb_setup.init) for one day or two, and then talk about mutual exclusion and updates in the sqlite case in depth

@Koncopd
Copy link
Member

Koncopd commented Nov 23, 2022

Mutual exclusion is not meaningful in this setting, because we don't get any changes from the cloud until new init call where new session is received.
I mean this change seems to break the sync logic totally.

@github-actions
Copy link

@github-actions github-actions bot temporarily deployed to pull request November 23, 2022 15:58 Inactive
@falexwolf falexwolf merged commit c945579 into main Nov 23, 2022
@falexwolf falexwolf deleted the session branch November 23, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants