Allow adding external v1 sst file with no global seqno support

[shuzhang1989]

This is a follow up fix for #1783. After it, we should be able to ingest external v1 sst files with no global seqno field.

[shuzhang1989]

can we make it into next stable release? also feels like this fix can be something to the release note in case people have the same problem

[shuzhang1989]

ping @siying @IslamAbdelRahman

[IslamAbdelRahman]

This does not look correct to me, we should never reach a point where we need to assign a global seqno with value != 0 when ingesting an external file with version = 1

The problem that we are trying to solve could be happening because when we get a V1 file we don't properly assign original_seqno value

[shuzhang1989]

thanks, so the situation is:

1. if i set opts.allow_global_seqno = true, the addfile will fail with "External SST file V1 does not support global seqno" (which is the logic from the previous PR)
2. if i set opts.allow_global_seqno = false, the addfile will fail with "Global seqno is required, but disabled".
3. I can confirm the SST is created by 4.11 sst file writer and i only added one key-value there. not sure how to set original_seqno there? does it get handled internally?
4. Logically, what if i have a dedicated check for v1 file in the code instead of putting it with other checks with global seq?

[IslamAbdelRahman]

@shuzhang1989 , What I meant is that we set the original_seqno here https://github.com/shuzhang1989/rocksdb/blob/57a584a0ea31ea1bd0ead0c5e21d562fcc14498a/db/external_sst_file_ingestion_job.cc#L302

We set it when reading a V2 file, but we don't when reading V1 .. So I think we should fix that

[shuzhang1989]

@IslamAbdelRahman, i wonder how the fix will be, because if version == 1, seqno_iter will be invalid, then what's the original_seqno to be set?
https://github.com/shuzhang1989/rocksdb/blob/57a584a0ea31ea1bd0ead0c5e21d562fcc14498a/db/external_sst_file_ingestion_job.cc#L311

[IslamAbdelRahman]

@shuzhang1989, In the V1 case we can simply set it to 0 since it's guaranteed that a V1 file will always have sequence number 0

file_to_ingest->original_seqno = 0;

[facebook-github-bot]

@shuzhang1989 updated the pull request - view changes

[shuzhang1989]

@IslamAbdelRahman thanks, verified it works 👍

[facebook-github-bot]

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[IslamAbdelRahman]

Thanks @shuzhang1989 !

[shuzhang1989]

@IslamAbdelRahman thanks a ton for the instruction!