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

kvserver: add merged SSTTimestampToRequestTimestamp param for AddSSTable #76110

Merged
merged 3 commits into from
Feb 21, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Feb 5, 2022

hlc: add Timestamp.IsSet()

This patch adds Timestamp.IsSet(), which is the inverse of
IsEmpty(). In complex boolean expressions, IsSet() is significantly
easier to parse than !IsEmpty(), which can avoid mistakes.

Release note: None

testutils/sstutil: tweak MakeSST() parameters

This makes testing.T the first parameter, as is customary, and
replaces the unnecessary Context parameter with a background context
as this is only used for logging.

Release note: None

kvserver: add merged UpdateToRequestTimestamp param for AddSSTable

This merges the AddSSTable parameters WriteAtRequestTimestamp and
SSTTimestamp into a new SSTTimestampToRequestTimestamp parameter.
This parameter specifies the MVCC timestamp of all existing timestamps
in the SST, which will be rewritten to the request timestamp if they
differ (e.g. if the request gets pushed).

Both of the replaced parameters are new in 22.1, so this does not
require a version gate and allows changing the type of one of the
existing Protobuf fields.

Release note: None

@erikgrinaker erikgrinaker requested a review from dt February 5, 2022 14:13
@erikgrinaker erikgrinaker requested review from a team as code owners February 5, 2022 14:13
@erikgrinaker erikgrinaker self-assigned this Feb 5, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch 2 times, most recently from 638261c to 193224e Compare February 5, 2022 14:57
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.

Reviewed 1 of 12 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt and @erikgrinaker)


pkg/roachpb/api.proto, line 1645 at r2 (raw file):

  bytes data = 2;

  // UpdateToRequestTimestamp updates all MVCC timestamps in the SST from the

(drive by comment. And the confusion may be limited to me so feel free to ignore.)

I found the "UpdateToRequestTimestamp updates all" and "parameter timestamp" phrasing a bit confusing. It also seemed that this was the timestamp to update to, when it is actually the timestamp in the sstable. How about something like:

// CurrentSSTableTimestampToRewrite when non-empty serves the following purpose:
// - It is the guaranteed timestamp of all entries in the sstable. This implies that the sstable only contains versioned keys.
// - It tells the proposal evaluator that if the request timestamp is not equal to this CurrentSSTableTimestampToRewrite, the evaluator must rewrite the sstable to the request timestamp.

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 @dt and @sumeerbhola)


pkg/roachpb/api.proto, line 1645 at r2 (raw file):

Previously, sumeerbhola wrote…

(drive by comment. And the confusion may be limited to me so feel free to ignore.)

I found the "UpdateToRequestTimestamp updates all" and "parameter timestamp" phrasing a bit confusing. It also seemed that this was the timestamp to update to, when it is actually the timestamp in the sstable. How about something like:

// CurrentSSTableTimestampToRewrite when non-empty serves the following purpose:
// - It is the guaranteed timestamp of all entries in the sstable. This implies that the sstable only contains versioned keys.
// - It tells the proposal evaluator that if the request timestamp is not equal to this CurrentSSTableTimestampToRewrite, the evaluator must rewrite the sstable to the request timestamp.

I like the concrete phrasing of the parameter's purpose. I don't love the name though. But names are hard. Maybe SSTTimestampToRequestTimestamp?

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt and @erikgrinaker)


pkg/roachpb/api.proto, line 1645 at r2 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

I like the concrete phrasing of the parameter's purpose. I don't love the name though. But names are hard. Maybe SSTTimestampToRequestTimestamp?

I'm fine with whatever name you come up with

@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch from 193224e to c059516 Compare February 11, 2022 20:47
@erikgrinaker erikgrinaker changed the title kvserver: add merged UpdateToRequestTimestamp param for AddSSTable kvserver: add merged SSTTimestampToRequestTimestamp param for AddSSTable Feb 11, 2022
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 @dt and @sumeerbhola)


pkg/roachpb/api.proto, line 1645 at r2 (raw file):

Previously, sumeerbhola wrote…

I'm fine with whatever name you come up with

Renamed the parameter and reworded the comment.

@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch from c059516 to 9025553 Compare February 18, 2022 08:19
@erikgrinaker
Copy link
Contributor Author

Pebble has been bumped, so this is good to go now.

@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch 3 times, most recently from 7ec382e to d01423d Compare February 19, 2022 12:28
This patch adds `Timestamp.IsSet()`, which is the inverse of
`IsEmpty()`. In complex boolean expressions, `IsSet()` is significantly
easier to parse than `!IsEmpty()`, which can avoid mistakes.

Release note: None
This makes `testing.T` the first parameter, as is customary, and
replaces the unnecessary `Context` parameter with a background context
as this is only used for logging.

Release note: None
@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch 2 times, most recently from cab0e29 to b384341 Compare February 20, 2022 20:10
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

Some nits about naming and comments. Otherwise seems good.

pkg/storage/sst.go Show resolved Hide resolved
pkg/roachpb/api.proto Outdated Show resolved Hide resolved
pkg/kv/kvserver/batcheval/cmd_add_sstable.go Show resolved Hide resolved
…STable`

This merges the `AddSSTable` parameters `WriteAtRequestTimestamp` and
`SSTTimestamp` into a new `SSTTimestampToRequestTimestamp` parameter.
This parameter specifies the MVCC timestamp of all existing timestamps
in the SST, which will be rewritten to the request timestamp if they
differ (e.g. if the request gets pushed).

Both of the replaced parameters are new in 22.1, so this does not
require a version gate and allows changing the type of one of the
existing Protobuf fields.

Release note: None
@erikgrinaker erikgrinaker force-pushed the addsstable-ssttimestamp-redux branch from b384341 to ef6a82b Compare February 21, 2022 12:07
@erikgrinaker
Copy link
Contributor Author

TFTR!

bors r=aliher1911

@craig
Copy link
Contributor

craig bot commented Feb 21, 2022

Build succeeded:

@craig craig bot merged commit 5c5b1e3 into cockroachdb:master Feb 21, 2022
@erikgrinaker erikgrinaker deleted the addsstable-ssttimestamp-redux branch March 4, 2022 14:01
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