-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: TestStoreRangeMergeRHSLeaseExpiration failed under stress #33656
Comments
SHA: https://github.com/cockroachdb/cockroach/commits/f5e3c29b2eed92868cf3d449575283e2e383f199 Parameters:
To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1089307&tab=buildLog
|
Repros immediately on #33566, doesn't repro on the previous commit. |
Doesn't repro with master and this diff: diff --git a/pkg/kv/txn_interceptor_heartbeat.go b/pkg/kv/txn_interceptor_heartbeat.go
index 2b133cc3a1..15ca50c299 100644
--- a/pkg/kv/txn_interceptor_heartbeat.go
+++ b/pkg/kv/txn_interceptor_heartbeat.go
@@ -178,7 +178,7 @@ func (h *txnHeartbeat) SendLocked(
ba.Txn.Key = anchor
}
- if !h.st.Version.IsActive(cluster.VersionLazyTxnRecord) {
+ if true || !h.st.Version.IsActive(cluster.VersionLazyTxnRecord) {
addedBeginTxn = true
// Set the key in the begin transaction request to the txn's anchor key. so my assumption would be that the merge txn writing its txn record lazily somehow causes this problem. @nvanbenschoten, could you take a look? |
I've tracked this down to a flake in the test and not an incompatibility between merges and omitted BeginTxns. The test installs a |
@tbg I'm running out of time to hack on this. I understand the problem well enough now to not be worried. It's a bug in the test, not in the code, so I think I'd feel comfortable skipping the test until I can resolve it. What do you think? |
SGTM, I can also take a look but since the weekend is coming up let's skip
it.
…On Fri, Jan 11, 2019, 18:56 Nathan VanBenschoten ***@***.*** wrote:
@tbg <https://github.com/tbg> I'm running out of time to hack on this. I
understand the problem well enough now to not be worried. It's a bug in the
test, not in the code, so I think I'd feel comfortable skipping the test
until I can resolve it. What do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33656 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE135O1tk4FYkdB6gU2qgARlWgMOxhV_ks5vCNBagaJpZM4Z64SB>
.
|
Informs cockroachdb#33656. I'm not aware of any places where this is necessary for correctness, but it is useful to prevent test flakes until tests are updated to expect lazy transaction record creation. Release note: None
👍 already reviewed. |
33674: kv: provide option to request eager txn record creation r=nvanbenschoten a=nvanbenschoten Informs #33656. I'm not aware of any places where this is necessary for correctness, but it is useful to prevent test flakes until tests are updated to expect lazy transaction record creation. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
Closes cockroachdb#33656. This fixes the root problem instead of sending BeginTxn requests to appease the test. This is an important change to make now because if we left merges using BeginTxn requests in 19.1 then we wouldn't be able to remove BeginTxn support in 19.2. Release note: None
Closes cockroachdb#33656. This fixes the root problem instead of sending BeginTxn requests to appease the test. This is an important change to make now because if we left merges using BeginTxn requests in 19.1 then we wouldn't be able to remove BeginTxn support in 19.2. Release note: None
34934: storage: stop sending BeginTxn requests in merge transactions, remove EagerRecord r=nvanbenschoten a=nvanbenschoten Closes #33656. This fixes the root problem instead of sending BeginTxn requests to appease the test. This is an important change to make now because if we left merges using BeginTxn requests in 19.1 then we wouldn't be able to remove BeginTxn support in 19.2. The PR then removes the EagerRecord transaction option entirely. 35019: stats: temporarily disable automatic stats r=rytaft a=rytaft This commit temporarily disables automatic statistics. We will reenable automtic stats once it's confirmed that #34928 fixes the performance issues. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Rebecca Taft <[email protected]>
SHA: https://github.com/cockroachdb/cockroach/commits/f5e3c29b2eed92868cf3d449575283e2e383f199
Parameters:
To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1089030&tab=buildLog
The text was updated successfully, but these errors were encountered: