Skip to content

sql: Fix test flake#35081

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
andy-kimball:flake
Feb 21, 2019
Merged

sql: Fix test flake#35081
craig[bot] merged 1 commit intocockroachdb:masterfrom
andy-kimball:flake

Conversation

@andy-kimball
Copy link
Copy Markdown
Contributor

Filter out PushTxn/ResolveIntent trace messages, since they represent intent
cleanup from earlier operations that interferes with subsequent tests. This
was already being done in other variations, so this commit extends that to
all the variations that are doing filtering of messages.

Fixes #34911

Release note: None

@andy-kimball andy-kimball requested a review from a team as a code owner February 20, 2019 01:29
@andy-kimball andy-kimball requested a review from a team February 20, 2019 01:29
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz
Copy link
Copy Markdown
Contributor

knz commented Feb 20, 2019 via email

@RaduBerinde
Copy link
Copy Markdown
Member

Can you take a look at #35093 and add a filter for SystemConfigSpan if you think it makes sense?

@RaduBerinde
Copy link
Copy Markdown
Member

Maybe these cases should select the logs we want rather than filtering out things we don't want?

Copy link
Copy Markdown
Contributor Author

@andy-kimball andy-kimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added SystemConfigSpan to the messages. In terms of only selecting logs we want, I'd think that could invalidate the test, because we might accidentally filter out extra statements that would indicate a bug.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz, @nvanbenschoten, and @RaduBerinde)

Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@tbg
Copy link
Copy Markdown
Member

tbg commented Feb 20, 2019

This is failing left and right, please merge this, thanks!
I'm noticing that CI for this PR failed on these same tests, though, so something is not quite fixed yet.

@RaduBerinde
Copy link
Copy Markdown
Member

Need to -rerwite the tests now that we filter out SystemConfigSpan.

Filter out PushTxn/ResolveIntent/SystemConfigSpan trace messages, since they
represent intent cleanup / system activity from earlier operations that
interferes with subsequent tests. This was already being done in other
variations, so this commit extends that to all the variations that are doing
filtering of messages.

Fixes cockroachdb#34911
Fixes cockroachdb#35093

Release note: None
@andy-kimball
Copy link
Copy Markdown
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Feb 21, 2019
35081: sql: Fix test flake r=andy-kimball a=andy-kimball

Filter out PushTxn/ResolveIntent trace messages, since they represent intent
cleanup from earlier operations that interferes with subsequent tests. This
was already being done in other variations, so this commit extends that to
all the variations that are doing filtering of messages.

Fixes #34911

Release note: None

35086: mvcc: don't write below provisional commit timestamp on WriteTooOld errors r=tbg a=nvanbenschoten

Fixes #34853.

This change fixes #34853, which appears to have been broken by
57d0201#diff-41e9e1b7829f8e25daeb564f42c26017L1514.
That change made it so that intents are now written at a transaction's
provisional commit timestamp instead of its original timestamp (which is
fantastic!). In doing so, it removed special-purpose code to forward the
`MVCCLogicalOpDetails` timestamp to the provisional commit timestamp of the
transaction because it assumed that the new `writeTimestamp` variable that the
change introduced already assumed this role.

Unfortunately, the change allowed this `writeTimestamp` variable to regress
below the transaction's provisional commit timestamp in subtle cases where a
committed value ended up between the transaction's original timestamp and its
provisional commit timestamp (a case that generates `WriteTooOldError`s). In
this case, we were replacing the `writeTimestamp` with the committed value's
timestamp + 1 logical tick. This regression allowed `MVCCWriteIntentOp`s to look
like they were ignoring closed timestamps.

The fix was to either introduce the special purpose code again or to avoid
letting `writeTimestamp` regress in this `WriteTooOldError` code path. This PR
chose the latter solution.

In doing so, this PR also improves upon the benefit already provided by #32487 -
it reduces the number of intents that need to be moved on transaction commit.
This is because it ensures that if a transaction has an original (read)
timestamp below a committed value and a provisional commit (write) timestamp
above a committed value, an intent left by that transaction will be in a
"committable" state without a need to rewrite it during intent resolution.

Release note: None

35112: kv: assert an absence of retries in TestTxnCoordSenderRetries  r=tbg,andreimatei a=nvanbenschoten

The test was only asserting that cases that should observe retries did, not the cases that should not observe retries did not.

The PR also fixes `TestPropagateTxnOnError`, which wasn't really testing anything. We now set txn.Writing on the client, so it wasn't verifying what it thought it was. We switch it to assert that observed timestamps are propagated on error instead.

35118: storage: Downgrade a spammy log message r=tbg a=bdarnell

This log message only occurs on clusters with multiple stores per
node, but on those clusters it fires multiple times per second.

Release note: None

Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Ben Darnell <ben@bendarnell.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 21, 2019

Build succeeded

@craig craig bot merged commit 09f611b into cockroachdb:master Feb 21, 2019
@andy-kimball andy-kimball deleted the flake branch February 23, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants