cache: Added consistent reads for cache#21428
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akstron The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @akstron. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
akstron
changed the title
akstron
changed the title
|
Please sign the DCO https://github.com/etcd-io/etcd/pull/21428/checks?check_run_id=65752362584 |
tests/integration/cache_test.go
Outdated
| func revLessThan(n int64) func(int64) bool { return func(r int64) bool { return r < n } } | ||
| func revGreaterEqual(n int64) func(int64) bool { return func(r int64) bool { return r >= n } } | ||
|
|
||
| func TestCacheConsistentRead(t *testing.T) { |
There was a problem hiding this comment.
the getTestCases also check for revision which is causing issue while trying to integrate. I want to send put requests outside the prefix to update the revision before every Get request but that also updates the server revision which is being checked in each test case.
example:
=== RUN TestCacheConsistentRead/fromKey_/foo/b
cache_test.go:638: revision: got 93, want 8
There was a problem hiding this comment.
That's good it's checking revision. That's the way
There was a problem hiding this comment.
Should I update getTestCases according to the new behaviour?
There was a problem hiding this comment.
Done. thanks. Also updated testWithPrefixGet test cases.
|
From https://github.com/etcd-io/etcd/pull/21428/checks?check_run_id=65752362584 |
akstron
force-pushed
the
dev/cache-consistent-read
branch
from
f532d80 to
e54ddc6
Compare
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 28 files with indirect coverage changes @@ Coverage Diff @@
## main #21428 +/- ##
==========================================
- Coverage 68.47% 68.33% -0.14%
==========================================
Files 428 428
Lines 35291 35291
==========================================
- Hits 24164 24116 -48
- Misses 9733 9769 +36
- Partials 1394 1406 +12 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
This is shaping good, just couple of nits, some testing and we should be good to merge. |
|
Please squash commits |
|
Run robustness tests with this PR in #21485, got linearization error. Looks like cache is still returning stale result in some case. 
Looks like issue is related to revision returned when Get has non zero revision provided. |
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
akstron
force-pushed
the
dev/cache-consistent-read
branch
from
ab469cf to
1f912eb
Compare
Signed-off-by: Alok Kumar Singh <dev.alok.singh123@gmail.com>
|
@akstron: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Please squash commits and keep the squashed into 1 commit |
| // waitTillRevision blocks until the local cache revision reaches rev, | ||
| // using the server's latest revision as the linearizable target. | ||
| func (c *Cache) waitTillRevision(ctx context.Context, rev int64) error { | ||
| if rev != 0 && c.store.LatestRev() >= rev { |
There was a problem hiding this comment.
Sorry my suggestion was wrong, we always need to read server Revision.
There was a problem hiding this comment.
Why would that be? Did you got that reason from the failing robustness test? I didn't got a chance to check it, let me check it
There was a problem hiding this comment.
Etcd server always linearizes read and thus returns linearized revision. Cache needs to mirror that behavior to be compatible.
| return nil | ||
| } | ||
|
|
||
| rev, err := c.linearizableRevision(ctx, rev) |
There was a problem hiding this comment.
Please move the function out of waitTillRevision to Get, it's unrelated to waiting till revision.
| return rev, nil | ||
| } | ||
|
|
||
| // waitTillRevision blocks until the local cache revision reaches rev, |
There was a problem hiding this comment.
Writing comments doest help by itself, usually it's a sign of code not being clean itself.
General rule: Comment should explain WHY something is being do e, not WHATis being done.
Don't write comments that just explain the function. Rather think how to improve the code to make it obvious
| if err := c.waitTillRevision(ctx, requestedRev); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
| if err := c.waitTillRevision(ctx, requestedRev); err != nil { | |
| return nil, err | |
| } | |
| serverRev, err := c.serverRevision(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if requestedRev > serverRev { | |
| return nil, rpctypes.ErrFutureRev | |
| } | |
| if err = c.waitTillRevision(ctx, serverRev); err != nil { | |
| return nil, err | |
| } |
| func (c *Cache) waitTillRevision(ctx context.Context, rev int64) error { | ||
| if rev != 0 && c.store.LatestRev() >= rev { | ||
| return nil | ||
| } | ||
|
|
||
| rev, err := c.linearizableRevision(ctx, rev) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
| func (c *Cache) waitTillRevision(ctx context.Context, rev int64) error { | |
| if rev != 0 && c.store.LatestRev() >= rev { | |
| return nil | |
| } | |
| rev, err := c.linearizableRevision(ctx, rev) | |
| if err != nil { | |
| return err | |
| } | |
| func (c *Cache) waitTillRevision(ctx context.Context, rev int64) error { |
|
Confirmed that with recommended changes the cache is linearizable |
3 participants
Cache supports consistent reads when IsSerializable is false. Based on: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2340-Consistent-reads-from-cache
Cache.Get Request Flow
Background: conditionalProgressRequestor.run(ctx)
The progress loop only sends
RequestProgresswhen at least onewaitTillRevisioncall is actively waiting, avoiding unnecessaryserver round-trips for serializable-only workloads.
Part of #19371