-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Make ingest external file backward compatible #1783
Conversation
@@ -291,24 +291,26 @@ Status ExternalSstFileIngestionJob::GetIngestedFileInfo( | |||
file_to_ingest->version = DecodeFixed32(version_iter->second.c_str()); | |||
|
|||
auto seqno_iter = uprops.find(ExternalSstFilePropertyNames::kGlobalSeqno); | |||
if (file_to_ingest->version == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep all these checks for V2, but add a special condition for V1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
return Status::Corruption("Was not able to find file global seqno field"); | ||
} | ||
} else { | ||
return Status::InvalidArgument("external file version is not supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep this as well (let's say we are ingesting V3 or V4, we should fail)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
dbf5be3
to
6c97993
Compare
@shuzhang1989 updated the pull request - view changes |
@IslamAbdelRahman updated |
@shuzhang1989 updated the pull request - view changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM .. just some minor comments
ingestion_options_.allow_global_seqno == true)) { | ||
return Status::InvalidArgument( | ||
"external file's version number is not correct"); | ||
} else if (supported_versions.find(file_to_ingest->version) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just keep this to be
else {
return Status::InvalidArgument("External SST file version is not supported");
}
I don't think that we need the supported_versions
list
} else if (file_to_ingest->version == 1 && | ||
(ingestion_options_.allow_blocking_flush == true || | ||
ingestion_options_.allow_global_seqno == true)) { | ||
return Status::InvalidArgument( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add an assert like this
// SST file V1 should not have global seqno field
assert(seqno_iter == uprops.end())
(ingestion_options_.allow_blocking_flush == true || | ||
ingestion_options_.allow_global_seqno == true)) { | ||
return Status::InvalidArgument( | ||
"external file's version number is not correct"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the error message to be something like this
"External SST file V1 does not support global seqno"
@shuzhang1989 updated the pull request - view changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! will land after tests pass
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
No description provided.