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

kv/kvserver: use proper formatting when debug printing intents #69809

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

AlexTalks
Copy link
Contributor

@AlexTalks AlexTalks commented Sep 3, 2021

This commit changes the formatting used when printing intents via the
CLI debug command from the default generated Protobuf formatter to our
custom MVCCMetadata formatter implementation. Additionally, the
MergeTimestamp and TxnDidNotUpdateMetadata fields have been added to
the output. This changes the debug formatting from the former
example:

0,0 /Local/RangeID/203/r/RangePriorReadSummary (0x0169f6cb727270727300): {Txn:<nil> Timestamp:0,0 Deleted:false KeyBytes:0 ValBytes:0 RawBytes:[230 123 85 161 3 10 12 10 10 8 146 229 195 204 139 135 186 208 22 18 12 10 10 8 146 229 195 204 139 135 186 208 22] IntentHistory:[] Me
rgeTimestamp:<nil> TxnDidNotUpdateMeta:<nil>}
/Local/Lock/Intent/Table/56/1/1319/6/3055/0 0361fea07d3f0d40ba8f44dc4ee46cbdc2 (0x017a6b12c089f705278ef70bef880001000361fea07d3f0d40ba8f44dc4ee46cbdc212): 1630559399.176502568,0 {Txn:id=61fea07d key=/Table/57/1/1319/6/0 pri=0.01718258 epo=0 ts=1630559399.176502568,0 min=1630559399.176502568,0 seq=4 Timestamp:1630559399.176502568,0 Deleted:false KeyBytes:12 ValBytes:5 RawBytes:[] IntentHistory:[] MergeTimestamp:<nil> TxnDidNotUpdateMeta:0xc0016059b0}

to the following example:

0,0 /Local/RangeID/203/r/RangePriorReadSummary (0x0169f6cb727270727300): txn={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/0x0a0c0a0a0892e5c3cc8b87bad016120c0a0a0892e5c3cc8b87bad016 mergeTs=<nil> txnDidNotUpdateMeta=false
/Local/Lock/Intent/Table/56/1/1319/6/3055/0 0361fea07d3f0d40ba8f44dc4ee46cbdc2 (0x017a6b12c089f705278ef70bef880001000361fea07d3f0d40ba8f44dc4ee46cbdc212): 1630559399.176502568,0 txn={id=61fea07d key=/Table/57/1/1319/6/0 pri=0.01718258 epo=0 ts=1630559399.176502568,0 min=1630559399.176502568,0 seq=4} ts=1630559399.176502568,0 del=false klen=12 vlen=5 mergeTs=<nil> txnDidNotUpdateMeta=true

Related to #69414

Release justification: Bug fix
Release note: None

@AlexTalks AlexTalks requested review from tbg and sumeerbhola September 3, 2021 02:56
@AlexTalks AlexTalks requested review from a team as code owners September 3, 2021 02:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Sep 3, 2021

Thanks Alex! CI is red, I think we want an update to TestFormatMVCCMetadata (which is nice, I was going to ask about testing) and the other tests probably just need a run with -rewrite.

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks and @erikgrinaker)


pkg/storage/enginepb/mvcc.go, line 323 at r1 (raw file):

		txnDidNotUpdateMeta = *meta.TxnDidNotUpdateMeta
	}
	fmt.Fprintf(buf, " mergeTs=%s txnDidNotUpdateMeta=%t", meta.MergeTimestamp, txnDidNotUpdateMeta)

will this print the LegacyTimestamp contents if MergeTimestamp is non-nil?

Copy link
Contributor Author

@AlexTalks AlexTalks 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! 1 of 0 LGTMs obtained (waiting on @erikgrinaker and @sumeerbhola)


pkg/storage/enginepb/mvcc.go, line 323 at r1 (raw file):

Previously, sumeerbhola wrote…

will this print the LegacyTimestamp contents if MergeTimestamp is non-nil?

Both the Timestamp and the MergeTimestamp fields are of type util.hlc.LegacyTimestamp, did you mean the first one? If so, that's already printed out, yes.

Copy link
Contributor Author

@AlexTalks AlexTalks 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 (and 1 stale) (waiting on @erikgrinaker and @sumeerbhola)


pkg/storage/enginepb/mvcc.go, line 323 at r1 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

Both the Timestamp and the MergeTimestamp fields are of type util.hlc.LegacyTimestamp, did you mean the first one? If so, that's already printed out, yes.

I'm not sure in what cases MergeTimestamp is non-nil, but if it's unnecessary in the output I can easily do one of:

  • skip printing if nil,
  • only print if expand=true (I.e. if printed like fmt.Printf("%+v", meta))
  • or just leave it as is

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @AlexTalks, @erikgrinaker, and @sumeerbhola)


pkg/storage/enginepb/mvcc.go, line 279 at r2 (raw file):

// String implements the fmt.Stringer interface.
func (meta *MVCCMetadata) String() string {

FWIW, this code and the methods below ought to be replaced by a SafeFormat method and associated wrappers.

This commit changes the formatting used when printing intents via the
CLI debug command from the default generated Protobuf formatter to our
custom `MVCCMetadata` formatter implementation.  Additionally, the
`MergeTimestamp` and `TxnDidNotUpdateMetadata` fields have been added to
the output.  This changes the debug formatting from the former
example:
```
0,0 /Local/RangeID/203/r/RangePriorReadSummary (0x0169f6cb727270727300): {Txn:<nil> Timestamp:0,0 Deleted:false KeyBytes:0 ValBytes:0 RawBytes:[230 123 85 161 3 10 12 10 10 8 146 229 195 204 139 135 186 208 22 18 12 10 10 8 146 229 195 204 139 135 186 208 22] IntentHistory:[] Me
rgeTimestamp:<nil> TxnDidNotUpdateMeta:<nil>}
/Local/Lock/Intent/Table/56/1/1319/6/3055/0 0361fea07d3f0d40ba8f44dc4ee46cbdc2 (0x017a6b12c089f705278ef70bef880001000361fea07d3f0d40ba8f44dc4ee46cbdc212): 1630559399.176502568,0 {Txn:id=61fea07d key=/Table/57/1/1319/6/0 pri=0.01718258 epo=0 ts=1630559399.176502568,0 min=1630559399.176502568,0 seq=4 Timestamp:1630559399.176502568,0 Deleted:false KeyBytes:12 ValBytes:5 RawBytes:[] IntentHistory:[] MergeTimestamp:<nil> TxnDidNotUpdateMeta:0xc0016059b0}
```
to the following example:
```
0,0 /Local/RangeID/203/r/RangePriorReadSummary (0x0169f6cb727270727300): txn={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/0x0a0c0a0a0892e5c3cc8b87bad016120c0a0a0892e5c3cc8b87bad016 mergeTs=<nil> txnDidNotUpdateMeta=false
/Local/Lock/Intent/Table/56/1/1319/6/3055/0 0361fea07d3f0d40ba8f44dc4ee46cbdc2 (0x017a6b12c089f705278ef70bef880001000361fea07d3f0d40ba8f44dc4ee46cbdc212): 1630559399.176502568,0 txn={id=61fea07d key=/Table/57/1/1319/6/0 pri=0.01718258 epo=0 ts=1630559399.176502568,0 min=1630559399.176502568,0 seq=4} ts=1630559399.176502568,0 del=false klen=12 vlen=5 mergeTs=<nil> txnDidNotUpdateMeta=true
```

Release justification: Bug fix
Release note: None
@AlexTalks
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 14, 2021

Build succeeded:

@craig craig bot merged commit 1f98510 into cockroachdb:master Sep 14, 2021
@AlexTalks AlexTalks deleted the intent_formatting branch October 7, 2021 02:50
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.

5 participants