kvserver: missing initialization of LAI tracking in the proposal buffer #60625
Labels
A-kv-replication
Relating to Raft, consensus, and coordination.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-kv
KV Team
Comments
andreimatei
added
C-bug
andreimatei
added a commit
to andreimatei/cockroach
that referenced
this issue
Feb 16, 2021
The initialization of the LAI tracking in the proposal buffer seems pretty lacking (see cockroachdb#60625). This patch adds initialization of propBuf.liBase at propBuf.Init() time, which is irrelevant for production, but will help future tests which will surely want the a propBuf's first assigned LAIs to have some relationship to the replica state. Release note: None
craig bot
pushed a commit
that referenced
this issue
Feb 18, 2021
59588: streamingest: add job level test for stream ingestion r=pbardea a=adityamaru This change adds a test that exercises all the components of the stream ingestion flow. It fixes some missing pieces that were discovered while writing the test. Informs: #59175 Release note: None 60424: sql: include sampled stats in TestSampledStatsCollection r=yuzefovich a=asubiotto Depends on #59992, which is required for this new regression test to pass. TestSampledStatsCollection would previously only check that stats that are collected regardless of the sample rate are returned. These types of stats (rows/bytes read) are propagated using metadata, rather than the trace. This resulted in us silently failing to collect any stats when sampling was enabled once the tracing mode was reverted back to legacy. To avoid this kind of thing happening again, this commit adds a check that max memory usage is reported to be non-zero. Release note: None (this is a new feature that has no user impact yet) 60626: kvserver: initialize propBuf LAI tracking r=andreimatei a=andreimatei The initialization of the LAI tracking in the proposal buffer seems pretty lacking (see #60625). This patch adds initialization of propBuf.liBase at propBuf.Init() time, which is irrelevant for production, but will help future tests which will surely want the a propBuf's first assigned LAIs to have some relationship to the replica state. Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
One would expect
propBuf.liBaseto be initialized on a new leaseholder when the lease changes hands. However, it doesn't appear to be. We seem to only synchronize thepropBuf'stracking with the replica state through this call, happening at the end of the firstFlush():https://github.com/cockroachdb/cockroach/blob/3b7dff158a796e03c83898f19ac512e519c3d128/pkg/kv/kvserver/replica_proposal_buf.go#L474
When a lease changes hands, I think we need to set the
liBase, and also to reset the buffer's counter (which gets added on top of theliBase), one way or another. And one way to do it is to force aFlush()immediately once the new lease applies.@nvanbenschoten said that he discovered the same thing a while back, and looked briefly into whether this problem results in the first batch of commands proposed after a lease change being rejected, and for some reason didn't see it happen. So maybe there's some hidden saving grace somewhere.
Jira issue: CRDB-3169
Epic CRDB-39898
The text was updated successfully, but these errors were encountered: