Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt 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 |
serathius
changed the title
|
Fixing the failed tests now. |
Signed-off-by: Nont <9658731+nwnt@users.noreply.github.com>
197d0b1 to
4dd8e29
Compare
|
Fixed the failed unit tests and lint. Looking at the e2e errors. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 24 files with indirect coverage changes @@ Coverage Diff @@
## main #22550 +/- ##
==========================================
- Coverage 73.01% 73.00% -0.02%
==========================================
Files 448 448
Lines 31813 31827 +14
==========================================
+ Hits 23227 23234 +7
- Misses 8583 8590 +7
Partials 3 3 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/retest |
|
@nwnt: The following tests 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. |
|
|
||
| func testKVGet(t *testing.T, stream bool) { | ||
| testRunner.BeforeTest(t) | ||
| for _, tc := range clusterTestCases() { |
There was a problem hiding this comment.
Hybrid version tests fail because we change behavior between minor version. This is expected as a fix for but that was not caught before.
Look how other tests filter clusterTestCases() for hybrid cases
|
Overall LGTM, we just need to disable upgrade tests and remember to reenable them after we backport fix. This is an important fix for regression in v3.7, after discussing it with Nont adding a lease to index is the only solution other disabling the optimization. As we already announced the optimization as part of the blogpost we expect people to depend on it, so we should consider backporting this change as a fix for v3.7. |
| Revisions(key, end []byte, atRev int64, limit int, withTotalCount bool) ([]Revision, int) | ||
| CountRevisions(key, end []byte, atRev int64) int | ||
| Put(key []byte, rev Revision) | ||
| Put(key []byte, leaseID lease.LeaseID, rev Revision) |
There was a problem hiding this comment.
It means that we need to store all leases in memory, also a lease for each revision of each key. We need to evaluate the impact on memory usage.
We don't have a user facing knob, the FastKeysOnly feature is always enabled. How much is the apiserver depend on this feature?
There was a problem hiding this comment.
Not 1.37, previous performance prevented any dependeence. We are planning to add some dependence in 1.38.
|
|
||
| type revisionLease struct { | ||
| revision Revision | ||
| leaseID lease.LeaseID |
There was a problem hiding this comment.
This means that even there is no lease, it also consumes 8 bytes for each revision
There was a problem hiding this comment.
Right, but even with K8s 1k QPS and 5 minute compaction window that's only ~2MB of data.
|
The way other way is to support |
|
The motivation of MVCC is to promote performance for concurrent read & write, but leases themselves aren't MVCCed. It's a little weird & pointless to couple leases with revisions inside the index . It would also make the design & implementation harder to evolve in future. Proposed solution:
|
This branch has not been deployed
4 participants
Ref: #22209
The change ensures that the lease associated with the revision is also returned, not just for the current revision but for the previous revisions when specified in the Range request.