spanconfig: implement the ProtectedTSReader interface on the KVSubscriber #77338
Conversation
arulajmani
requested review from
ajwerner,
nvanbenschoten,
irfansharif and
adityamaru
|
This change is |
arulajmani
force-pushed
the
kv-subscriber-pts-reader
branch
6 times, most recently
from
65fc4ac to
5092a10
Compare
arulajmani
force-pushed
the
kv-subscriber-pts-reader
branch
from
5092a10 to
d37617f
Compare
|
@irfansharif made the changes you mentioned on Slack, namely, removed |
arulajmani
force-pushed
the
kv-subscriber-pts-reader
branch
from
d37617f to
0300001
Compare
There was a problem hiding this comment.
LGTM modulo [e,e) showing up in the test grammar to represent a point span. It’s an invalid span if e is both inclusive and exclusive. In our test I think we’ll get the same coverage just testing for the single alphabet span [e,f). If you really want to test keys (not necessary IMO, KV doesn’t ask for configs for keys, does it?) it’s worth adding something in the testing grammar to represent it. Either [e,nil) or some syntax for point specifically. I prefer [e,f) also because at various places we assert on spans having non-nil end keys.
| canGC, _, gcTimestamp, oldThreshold, newThreshold := r.checkProtectedTimestampsForGC(ctx, makeTTLDuration(10)) | ||
| canGC, _, gcTimestamp, oldThreshold, newThreshold, err := r.checkProtectedTimestampsForGC(ctx, makeTTLDuration(10)) | ||
| require.NoError(t, err) | ||
| require.True(t, canGC) |
This patch makes the `KVSubscriber` implement the `ProtectedTSReader` interface. This allows the GC queue to consult protected timestamp information reconciled by sql tenants when making GC decisions. We also initialize the `spanconfigptsreader.adapter` struct with a KVSubscriber now. The adapter was conceived with the intention to encapsulate PTS information from both the old and new subsystem for release 22.1 behind a single source for the GC queue to consult. Threading in the KVSubscriber actually achieves this. We return the minimum freshness of the cache and KVSubscriber to the GC queue as the "combined" freshnsess of the adapter. Release justification: bug fixes and low risk updates to new functionality Release note: None
arulajmani
force-pushed
the
kv-subscriber-pts-reader
branch
from
0300001 to
63fde7a
Compare
There was a problem hiding this comment.
TFTR! I removed the point tests considering KV doesn't ask for them and given your comment. The CI failure seems to be a flake, going to press merge.
bors r+
Dismissed @irfansharif from a discussion.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @nvanbenschoten)
pkg/kv/kvserver/replica_protected_timestamp_test.go, line 155 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Duplicate line?
Done.
|
Build succeeded: |
This patch makes the
KVSubscriberimplement theProtectedTSReaderinterface. This allows the GC queue to consult protected timestamp
information reconciled by sql tenants when making GC decisions.
We also initialize the
spanconfigptsreader.adapterstruct with aKVSubscriber now. The adapter was conceived with the intention to
encapsulate PTS information from both the old and new subsystem for
release 22.1 behind a single source for the GC queue to consume.
Threading in the KVSubscriber actually achieves this. We return the
minimum freshness of the cache and KVSubscriber to the GC queue from
the adapter.
Release justification: bug fixes and low risk updates to new
functionality
Release note: None