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

Update default rqlite version to 8.23.0 #19

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Conversation

otoolep
Copy link
Member

@otoolep otoolep commented Mar 14, 2024

No description provided.

@otoolep otoolep changed the title Bump to SQLite 8.23.0 Bump to rqlite 8.23.0 Mar 14, 2024
@otoolep
Copy link
Member Author

otoolep commented Mar 14, 2024

@jtackaberry

@jtackaberry jtackaberry changed the title Bump to rqlite 8.23.0 Update default rqlite version to 8.23.0 Mar 15, 2024
@jtackaberry jtackaberry self-assigned this Mar 15, 2024
@jtackaberry jtackaberry merged commit 1f2cfb7 into master Mar 15, 2024
@jtackaberry
Copy link
Collaborator

Thanks @otoolep !

Looking at the new sync query param for /readyz, it seems fairly safe and reasonable to use this for a startup probe (i.e. not take any traffic initially until /readyz?sync returns 200 just to ensure things have caught up during initial startup. But as an ongoing readiness probe once traffic starts flowing in, it's not clear if this is wise. After all, the client can dictate the read consistency.

What do you think? Maybe you could expand a bit on the use case(s) that motivated this enhancement?

@jtackaberry jtackaberry deleted the otoolep-8.23.0 branch March 15, 2024 00:11
@otoolep
Copy link
Member Author

otoolep commented Mar 15, 2024

Yeah, I wouldn't change readiness probes. sync is for specific needs, which folks might have.

I introduced sync while helping users who had large snapshots transferring between nodes, and were having a hard time knowing when the transfer was complete. The change didn't really help, but the work was done, so I went ahead and added it. strict_freshness (new for Read consistency control) ended up being more effective.

rqlite/rqlite#1672 -- contains a lot of the discussion.

@jtackaberry
Copy link
Collaborator

Thanks for the link. (Somehow I missed that when skimming the PR.)

Reading (more or less :)) through the discussion I can see a benefit to adding a startup probe that includes the new sync flag, just to make sure a node is fully up-to-date before it begins handling requests. Will work up a PR for that.

@otoolep
Copy link
Member Author

otoolep commented Mar 15, 2024

Reading (more or less :)) through the discussion I can see a benefit to adding a startup probe that includes the new sync flag, just to make sure a node is fully up-to-date before it begins handling requests.

Unless that node is going to serve queries with none Read Consistency, and some sort of freshness interval, there isn't much point though, as all reads will go back the Leader anyway. As long as the node is part of the cluster, it can service reads. It doesn't have to be "caught up".

It was with read-only (non-voting) nodes that kicked this whole thing off.

@otoolep
Copy link
Member Author

otoolep commented Mar 15, 2024

Honestly, I don't think I'd bother. It just delays the node being "ready", for little reason. Again, unless the user knows what they are doing, and have a special read-only use case, with none consistency.

@jtackaberry
Copy link
Collaborator

jtackaberry commented Mar 15, 2024

Honestly, I don't think I'd bother. It just delays the node being "ready", for little reason.

Yeah, I see your point. Fair enough, will leave things as they are. Thanks!

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.

2 participants