sql: add ImportType to descriptor #120809
Conversation
stevendanna
requested review from
dt and
mgartner
and removed request for
a team
|
This change is |
|
First commit is #120407 |
| // Next ID: 60 | ||
| // ImportType is the type of import that is running if | ||
| // ImportStartWallTime is set. | ||
| optional ImportType import_type = 60 [(gogoproto.nullable) = false, (gogoproto.customname) = "ImportType"]; |
There was a problem hiding this comment.
could this be a bool instead with the var name epoch_based_import_runnning? This feels cleaner then import_type==0 (i.e. IMPORT_WITH_START_TIME_ONLY) implying no import running or non-epoch based import.
There was a problem hiding this comment.
I went back and forth on this. At the time it seemed likely that we might end up with a few more types if we eventually want to differentiate between empty-vs-non-empty as well like you had in your original PR. If so, I'd slightly prefer an Enum or a uint64 with different bits assigned to different properties of the import.
But, I can change it to a bool.
|
it may also be worth adding a small unit test for this |
stevendanna
force-pushed
the
ssd/import-type
branch
from
b02d039 to
3ff932e
Compare
| // Next ID: 60 | ||
| // ImportType is the type of import that is running if | ||
| // ImportStartWallTime is set. | ||
| optional ImportType import_type = 60 [(gogoproto.nullable) = false, (gogoproto.customname) = "ImportType"]; |
|
closing in favor of #121042 |
…tting Currently, the user can disable import from writing the import epoch to the mvcc value header. This cluster setting was added because we thought this import epoch logic could be risky: - it could degrade import performance - epoch based rollbacks could be incorrect and much slower The second risk isn't serious because only online restore, a feature in private preview, uses epoch based rollback. The first risk does not seem apparent according to import roachperf graphs. The cluster setting further complicates the rollback code, as pr cockroachdb#120809 shows. Since the risks of this code aren't terribly bad, it seems fine to remove the code to simplify the import rollback logic. Epic: none Release note: none
|
Changed my mind. I'm reviving this given my comment |
msbutler
force-pushed
the
ssd/import-type
branch
from
3ff932e to
083b8ce
Compare
There was a problem hiding this comment.
Reviewed 4 of 6 files at r2, 1 of 10 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @stevendanna)
This adds an ImportType field to table descriptors. We need this field if we want to support disabling ImportEpoch writing via a cluster setting since we won't necessarily be able to trust that a non-zero ImportEpoch is actually from the currently in-progress IMPORT. Epic: none Release note: None
msbutler
force-pushed
the
ssd/import-type
branch
from
083b8ce to
0347bef
Compare
|
bors r=dt |
|
TFTRs! bors r+ |
|
Already running a review |
|
Build failed (retrying...): |
This adds an ImportType field to table descriptors. We need this field
if we want to support disabling ImportEpoch writing via a cluster
setting since we won't necessarily be able to trust that a non-zero
ImportEpoch is actually from the currently in-progress IMPORT.
Epic: none
Release note: None