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

Support checking Stale Reads relative to Leader clock #1681

Merged
merged 14 commits into from
Feb 13, 2024
Merged

Conversation

otoolep
Copy link
Member

@otoolep otoolep commented Feb 10, 2024

No description provided.

Copy link

@aderouineau aderouineau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should /readyz be updated to allow freshness check? Strictly speaking I don't think it's necessary as an application could send a SELECT 1 with the strict freshness requirement. But it may be a nice convenience?

store/store.go Outdated Show resolved Hide resolved
store/store.go Show resolved Hide resolved
@@ -36,6 +36,7 @@ message QueryRequest {
}
Level level = 3;
int64 freshness = 4;
int64 max_stale = 5;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about re-using Level, introducing a QUERY_REQUEST_LEVEL_FRESH? Or as an alternative, have a strict_fresh boolean.

I am not for another timestamp because max_stale implies freshness, and is conceptually a stricter freshness.

Copy link
Member Author

@otoolep otoolep Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the goal? I don't see what you're getting at. What meaning are you trying to communicate with strict_fresh?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Corrected review comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUERY_REQUEST_LEVEL_FRESH / strict_fresh means that you not only want time.Since(LeaderLastContact).Nanoseconds() > Freshness, but also want FSMIndex < CommitIndex && LastFSMUpdateTime.Sub(LastAppendedAtTime).Nanoseconds() > MaxStale where MaxStale would instead be freshness

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let me think about it, it might be simpler to understand. I don't want to add a new consistency level however, because today consistency levels have nothing to do with time -- it's an orthogonal concept.

But perhaps a simple strict flag, as an optional flag to include with freshness might be better. That way the user doesn't have to think about specifying two durations. Or I just add a new, standalone duration parameter, called freshness_strict (or something).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to add a new consistency level however, because today consistency levels have nothing to do with time -- it's an orthogonal concept.

I don't think there's a definition that says they do not have to abide by time rules. Quoting https://en.wikipedia.org/wiki/Consistency_(database_systems):

consistency (or correctness) refers to the requirement that any given database transaction must change affected data only in allowed ways. Any data written to the database must be valid according to all defined rules, including constraints, cascades, triggers, and any combination thereof.

And I believe from the point of view of a user, QUERY_REQUEST_LEVEL_FRESH makes sense because it means they want a higher degree of trust (consistency). Granted, the one thing that makes it strange is that it's only applied if freshness is specified in the query.

However, I am not against having a boolean query parameter that strengthens the freshness check. I am just trying to justify why a new consistency level could make sense. I believe I could even argue that "freshness" could have been a parameterized level of consistency instead of a query parameter, as it itself is a level of consistency above "NONE".

perhaps a simple strict flag

I figured strict_fresh because strict is pretty generic and there may be uses for that in the future.

Copy link
Member Author

@otoolep otoolep Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a definition that says they do not have to abide by time rules.

As the designer of rqlite, I have a certain discretion to decide what the term consistency involves for my database. :-)

rqlite's use of the term consistency hasn't involved a time dimension up to now, and I do not want to mix logical concepts (consistency) with temporal concepts. In other words there is a pattern to rqlite's use of the terms now, I believe the separation works well, and I don't want to introduce unnecessary change.

I figured strict_fresh because strict is pretty generic and there may be uses for that in the future.

I would go with freshness_strict, which makes it clear that's it's related, but we're starting to bikeshed now. I recognize that freshness is fundamentally not a great term, but it's more disruptive (and a breaking change) to try to change it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment.

@otoolep
Copy link
Member Author

otoolep commented Feb 10, 2024

Should /readyz be updated to allow freshness check? Strictly speaking I don't think it's necessary as an application could send a SELECT 1 with the strict freshness requirement. But it may be a nice convenience?

Maybe, but one change at a time. :-) Let's see if we can agree on this proposed change, and gain confidence that it is correct.

store/state.go Outdated Show resolved Hide resolved
@otoolep
Copy link
Member Author

otoolep commented Feb 10, 2024

Thanks again @aderouineau -- I think we're getting close here. Letting it sit for a few days, while we think about the consequences of this change will definitely help. Ongoing feedback welcome.

@otoolep
Copy link
Member Author

otoolep commented Feb 12, 2024

Try a simple, optional, boolean flag freshness_strict. An example query would be:

curl 'localhost:4001/db/query?level=none&freshness=1s&freshness_strict' --data-urlencode 'q=SELECT * FROM foo'

@otoolep otoolep merged commit cecc147 into master Feb 13, 2024
19 checks passed
@otoolep otoolep deleted the stale-leader-time branch February 13, 2024 00:34
@otoolep
Copy link
Member Author

otoolep commented Feb 13, 2024

Available in 8.20.0.

Thanks again, @aderouineau

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