Skip to content
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

rangefeed: add non-fatal assertion for intent commit resolved ts #118196

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jan 23, 2024

This patch adds a non-fatal assertion for intent commit operations that are emitted below the resolved timestamp. A known bug will do just this, so the assertion is kept non-fatal to avoid disruption, but it will confirm whether a cluster is hitting the bug.

The log messages are of the form:

E240123 10:49:00.052256 57 kv/kvserver/rangefeed/resolved_timestamp.go:162  [n1,s1,r7/1:‹/Table/{3-4}›,rangefeed] 1  resolved timestamp 0.000000024,0 equal to or above timestamp of operation txn_id:a55c0992-581b-4cb2-97bf-fd9e22f7a657 key:"foo" timestamp:<wall_time:45 > value:"bar"

Touches #104309.
Touches #117612.
Epic: none
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)


pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 161 at r1 (raw file):

		// A known bug will trip this assertion, so don't fatal to avoid disruption.
		// See: https://github.com/cockroachdb/cockroach/issues/104309
		if t.Timestamp.LessEq(rts.resolvedTS) {

Should we expand assertOpAboveRTS to take a context and a fatal bool flag, then have it call either log.Fatalf or log.Errorf accordingly, then just call that here?

This patch adds a non-fatal assertion for intent commit operations that
are emitted below the resolved timestamp. A known bug will do just this,
so the assertion is kept non-fatal to avoid disruption, but it will
confirm whether a cluster is hitting the bug.

The log messages are of the form:

```
E240123 10:49:00.052256 57 kv/kvserver/rangefeed/resolved_timestamp.go:162  [n1,s1,r7/1:‹/Table/{3-4}›,rangefeed] 1  resolved timestamp 0.000000024,0 equal to or above timestamp of operation txn_id:a55c0992-581b-4cb2-97bf-fd9e22f7a657 key:"foo" timestamp:<wall_time:45 > value:"bar"
```

Epic: none
Release note: None
@erikgrinaker erikgrinaker force-pushed the 22.2.17-rangefeed-intent-assertion branch from 4ddc7ae to b6265f7 Compare January 23, 2024 19:25
@erikgrinaker erikgrinaker changed the title rangefeed: add non-fatal assertion for commit intent resolve ts rangefeed: add non-fatal assertion for intent commit resolved ts Jan 23, 2024
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

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


pkg/kv/kvserver/rangefeed/resolved_timestamp.go line 161 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we expand assertOpAboveRTS to take a context and a fatal bool flag, then have it call either log.Fatalf or log.Errorf accordingly, then just call that here?

If we were going to actually merge this, then yes. Since this is a one-off custom build, I'll just keep it as is.

Will address this over on #117612.

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.

4 participants