kvcoord: condense read spans when they exceed the memory limit #46275
Conversation
|
This change is |
There was a problem hiding this comment.
For the release note:
- I think you need to give it a category
- s/Transsactions/Transactions/
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 7 of 7 files at r3, 5 of 5 files at r4, 2 of 2 files at r5, 11 of 11 files at r6.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go, line 1678 at r6 (raw file):
}, { // If we've exhausted the limit for tracking refresh spans but we
We should try to keep these tests somehow. We don't want to lose testing coverage, even if we expect the number of cases where the refresh spans become invalid to decrease.
pkg/kv/kvclient/kvcoord/range_iter.go, line 206 at r4 (raw file):
// Check for an early exit from the retry loop. if pErr := ri.ds.deduceRetryEarlyExitError(ctx); pErr != nil {
If you change deduceRetryEarlyExitError to return an error, which looks possible without any casts, then you won't need any GoError() calls, which are always a bit suspicious.
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 220 at r6 (raw file):
// TxnCoordSender's wrapped sender. First, each of the interceptor objects // is initialized. var riGen rangeIteratorFactory
Why isn't this in initCommonInterceptors?
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 890 at r5 (raw file):
// The method has the side effect of sorting the stable write set. func (s *condensableSpanSet) mergeAndSort() (distinct bool) { oldLen := len(s.s)
Is it worth writing a test for this fix? It shouldn't be too much work given how close we're are to this code in txn_interceptor_pipeliner_test.go.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 181 at r6 (raw file):
type txnPipeliner struct { st *cluster.Settings riGen rangeIteratorFactory //used to condense lock spans, if provided
"// used"
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 207 at r6 (raw file):
} // condensableSpanSetRangeIterator describes the interface of RangeIterator
Does this belong in txn_interceptor_pipeliner.go? If the rangeIteratorFactory and condensableSpanSet are used in multiple interceptors then I'd put them either a) in their own file or b) in txn_coord_sender.go.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 220 at r6 (raw file):
// lazily. It's used to avoid allocating an iterator when it's not needed. The // factory can be configured either with a callback, used for mocking in tests, // or with a DistSender. Can also not be configured with anything, in which
This sentence is missing an ending.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 920 at r6 (raw file):
} func (s *condensableSpanSet) insertMultiple(spans []roachpb.Span) {
Might as well add a comment here like we have for insert.
Also, any reason not to use a variadic arg?
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 951 at r6 (raw file):
// spans within the same range boundaries are also condensed. // // Returns true if condensing was necessary.
Right now it returns true if condensing was performed, not if it was necessary. Should we return true from the "failed to condense lock spans" case or update the comment?
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 1048 at r6 (raw file):
func (s *condensableSpanSet) clear() { s.s = nil s.bytes = 0
s.condensed = false?
Or just *s = condensableSpanSet{} to avoid this kind of bug.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go, line 563 at r6 (raw file):
tp, mockSender := makeMockTxnPipeliner() // Disable maxInFlightSize, maxBatchSize and max_intents_bytes limits.
", and"
Also, could you make this either all refer to the setting names or all refer to the variable names?
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 109 at r5 (raw file):
wrapped lockedSender // refreshSpans contains key spans which were read during the transaction. In
Why remove this comment?
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 110 at r6 (raw file):
refreshFootprint condensableSpanSet riGen rangeIteratorFactory
Move this above wrapped. It's a dependency of the struct.
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 115 at r6 (raw file):
// Although maybe we should still be using it when // condensing the spans doesn't reduce them as much as we want.
I'm surprised to find that we aren't. Why is that?
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 338 at r6 (raw file):
refreshSpanBa := roachpb.BatchRequest{} refreshSpanBa.Txn = refreshTxn addRefreshes := func(refreshes condensableSpanSet) {
nit: it feels more natural for this to be a pointer.
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go, line 772 at r6 (raw file):
require.Equal(t, hlc.Timestamp{}, tsr.refreshedTimestamp) // Send a batch above the limit.
It's unfortunate that we're losing this test coverage as well.
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 76 at r6 (raw file):
} metaRefreshSpanBytesExceeded = metric.Metadata{ Name: "txn.refreshspanbytesexceeded",
Does this metric make sense anymore? Now that exceeding the byte limit does not imply auto-failing, should we break it into:
- a metric about the # txns that exceeded byte limit
- a metric about the # txns that failed to refresh
pkg/roachpb/merge_spans.go, line 40 at r5 (raw file):
// The input spans are not safe for re-use. // // Note that even if it returns true, adjacent spans might have been merged
nit: move this right after the sentence about the return value.
andreimatei
force-pushed
the
kv.refresh-span-collapse
branch
2 times, most recently
from
ae6c819 to
34dd44e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go, line 1678 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We should try to keep these tests somehow. We don't want to lose testing coverage, even if we expect the number of cases where the refresh spans become invalid to decrease.
ok, I've added a knob to disable condensing and added back these tests
pkg/kv/kvclient/kvcoord/range_iter.go, line 206 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
If you change
deduceRetryEarlyExitErrorto return anerror, which looks possible without any casts, then you won't need anyGoError()calls, which are always a bit suspicious.
done
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 220 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why isn't this in
initCommonInterceptors?
moved
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 890 at r5 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Is it worth writing a test for this fix? It shouldn't be too much work given how close we're are to this code in
txn_interceptor_pipeliner_test.go.
done
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 181 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
"// used"
done
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 207 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Does this belong in
txn_interceptor_pipeliner.go? If the rangeIteratorFactory andcondensableSpanSetare used in multiple interceptors then I'd put them either a) in their own file or b) intxn_coord_sender.go.
moved to a new file in a new commit
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 220 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This sentence is missing an ending.
done
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 920 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Might as well add a comment here like we have for
insert.Also, any reason not to use a variadic arg?
switch to variadic arg
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 951 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Right now it returns true if condensing was performed, not if it was necessary. Should we return true from the "failed to condense lock spans" case or update the comment?
updated comment
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 1048 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
s.condensed = false?Or just
*s = condensableSpanSet{}to avoid this kind of bug.
done, good catch
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go, line 563 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
", and"
Also, could you make this either all refer to the setting names or all refer to the variable names?
done
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 109 at r5 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Why remove this comment?
reinstated
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 110 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Move this above
wrapped. It's a dependency of the struct.
done
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 115 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
// Although maybe we should still be using it when // condensing the spans doesn't reduce them as much as we want.I'm surprised to find that we aren't. Why is that?
done
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 338 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: it feels more natural for this to be a pointer.
done
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go, line 772 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
It's unfortunate that we're losing this test coverage as well.
put it back
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 76 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Does this metric make sense anymore? Now that exceeding the byte limit does not imply auto-failing, should we break it into:
- a metric about the # txns that exceeded byte limit
- a metric about the # txns that failed to refresh
see now
pkg/roachpb/merge_spans.go, line 40 at r5 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: move this right after the sentence about the return value.
done
There was a problem hiding this comment.
This is close to an 
Reviewed 19 of 19 files at r7, 1 of 1 files at r8, 7 of 7 files at r9, 5 of 5 files at r10, 3 of 3 files at r11, 4 of 4 files at r12, 14 of 14 files at r13.
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go, line 1760 at r13 (raw file):
// error. This is the case even if the final batch itself causes a // refresh. name: "forwarded timestamp with too many refreshes in batch commit triggering refresh",
What's up with this spacing? Does this need a gofmt?
pkg/kv/kvclient/kvcoord/range_iter.go, line 203 at r13 (raw file):
// Check for an early exit from the retry loop. if err := ri.ds.deduceRetryEarlyExitError(ctx); err != nil {
nit: do the intermediate commits all compile?
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 181 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
done
Is this done?
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 109 at r5 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
reinstated
Was it?
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 338 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
done
Is it?
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 75 at r13 (raw file):
metaRefreshSuccess = metric.Metadata{ Name: "txn.refresh.success", Help: "Number of successful refreshes.",
Do these "Help" texts usually have punctuation? It doesn't look like it.
This patch removes a scary comment and some code that has become unnecessary a while ago. It had to do with protecting some state that's no longer shared. Release note: None
Trouble with range iteration can cause a transaction to fail to collapse its write footprint. This is unexpected enough, and also not handled well, so let's Warningf. Particularly since we're about to start using this code to collapse the read footprint too. Release note: None
It's needed by the next commit. Release note: None
The range iterator used to return pErr. This patch changes it to regular error. The RangeIter is used more and more outside of the DistSender. Almost all the callers already wanted regular errors. There was no benefit to returning pErr from it. More over, the next commit want to abstract an interface from it, and so pErr really doesn't belong. Release note: None
The span set was misinterpreting the return of roachpb.MergeSpans(). Release note: None
... in anticipation of it being used by a second interceptor. Release note: None
andreimatei
force-pushed
the
kv.refresh-span-collapse
branch
from
34dd44e to
7320018
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go, line 1760 at r13 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What's up with this spacing? Does this need a
gofmt?
well, the formatter aligns it with the next field - which has a long name. I've done something.
pkg/kv/kvclient/kvcoord/range_iter.go, line 203 at r13 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: do the intermediate commits all compile?
they do. It's not many people that I would have checked for.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go, line 181 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Is this done?
now it is... I had stashed some stuff and never put it back...
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 109 at r5 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Was it?
is now
pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go, line 338 at r6 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Is it?
now it is...
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 75 at r13 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do these "Help" texts usually have punctuation? It doesn't look like it.
fixed
There was a problem hiding this comment.
Reviewed 4 of 7 files at r16, 3 of 5 files at r17, 1 of 3 files at r18, 1 of 4 files at r19, 14 of 14 files at r20.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 75 at r13 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
fixed
Same issue below.
Before this patch, once a transaction exceeds the kv.transaction.max_refresh_spans_bytes limit, it stopped tracking reads and it didn't attempt to refresh any more when pushed. This patch make the span refresher condense the spans when it runs out of memory instead. So we'll get bigger spans and potentially false conflicts, but at least we have a chance at refreshing. In particular, it'll succeed if there's no writes anywhere. The condensing is performed using the condensableSpanSet, like we do in the pipeliner interceptor for the tracking of write intents. Internally, that guy condenses spans in ranges with lots of reads. We've seen people run into kv.transaction.max_refresh_spans_bytes in the past, so this should help many uses cases. But in particular I've written this patch because, without it, I'm scared about the effects of 20.1's reduction in the closed timestamp target duration to 3s from a previous 30s. Every transaction writing something after having run for longer than that will get pushed, so being able to refresh is getting more important. Fixes cockroachdb#46095 Release note (general change): Transactions reading a lot of data behave better when exceeding the memory limit set by kv.transaction.max_refresh_spans_bytes. Such transactions now attempt to resolve the conflicts they run into instead of being forced to always retry. Increasing kv.transaction.max_refresh_spans_bytes should no longer be necessary for most workloads. Release justification: fix for new "functionality" - the reduction in the closed timestamp target duration.
andreimatei
force-pushed
the
kv.refresh-span-collapse
branch
from
7320018 to
d230743
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_metrics.go, line 75 at r13 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Same issue below.
fixed.
Build succeeded |
|
Reminder to backport this to |
46803: release-20.1: kvcoord: condense read spans when they exceed the memory limit r=andreimatei a=andreimatei Backport 7/7 commits from #46275. /cc @cockroachdb/release --- Before this patch, once a transaction exceeds the kv.transaction.max_refresh_spans_bytes limit, it stopped tracking reads and it didn't attempt to refresh any more when pushed. This patch make the span refresher condense the spans when it runs out of memory instead. So we'll get bigger spans and potentially false conflicts, but at least we have a chance at refreshing. In particular, it'll succeed if there's no writes anywhere. The condensing is performed using the condensableSpanSet, like we do in the pipeliner interceptor for the tracking of write intents. Internally, that guy condenses spans in ranges with lots of reads. We've seen people run into kv.transaction.max_refresh_spans_bytes in the past, so this should help many uses cases. But in particular I've written this patch because, without it, I'm scared about the effects of 20.1's reduction in the closed timestamp target duration to 3s from a previous 30s. Every transaction writing something after having run for longer than that will get pushed, so being able to refresh is getting more important. Fixes #46095 Release note (general change): Transactions reading a lot of data behave better when exceeding the memory limit set by kv.transaction.max_refresh_spans_bytes. Such transactions now attempt to resolve the conflicts they run into instead of being forced to always retry. Increasing kv.transaction.max_refresh_spans_bytes should no longer be necessary for most workloads. Release justification: fix for new "functionality" - the reduction in the closed timestamp target duration. Co-authored-by: Andrei Matei <[email protected]>
Before this patch, once a transaction exceeds the
kv.transaction.max_refresh_spans_bytes limit, it stopped tracking reads
and it didn't attempt to refresh any more when pushed.
This patch make the span refresher condense the spans when it runs out
of memory instead. So we'll get bigger spans and potentially false
conflicts, but at least we have a chance at refreshing. In particular,
it'll succeed if there's no writes anywhere.
The condensing is performed using the condensableSpanSet, like we do in
the pipeliner interceptor for the tracking of write intents. Internally,
that guy condenses spans in ranges with lots of reads.
We've seen people run into kv.transaction.max_refresh_spans_bytes in the
past, so this should help many uses cases. But in particular I've
written this patch because, without it, I'm scared about the effects of
20.1's reduction in the closed timestamp target duration to 3s from a
previous 30s. Every transaction writing something after having run for
longer than that will get pushed, so being able to refresh is getting
more important.
Fixes #46095
Release note (general change): Transactions reading a lot of data behave
better when exceeding the memory limit set by
kv.transaction.max_refresh_spans_bytes. Such transactions now attempt to
resolve the conflicts they run into instead of being forced to always
retry. Increasing kv.transaction.max_refresh_spans_bytes should no
longer be necessary for most workloads.
Release justification: fix for new "functionality" - the reduction in
the closed timestamp target duration.