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

Feature: Ingest External SST #188

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

wangnengjie
Copy link
Contributor

@wangnengjie wangnengjie commented Aug 23, 2022

Signed-off-by: wangnengjie 751614701@qq.com

See details in: #187

Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
…heck test

Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
@wangnengjie
Copy link
Contributor Author

The next_txn_ts won't update when reopening db? @GanZiheng

Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
@GanZiheng
Copy link
Contributor

GanZiheng commented Aug 24, 2022

The next_txn_ts won't update when reopening db? @GanZiheng

Yes... I remember it is set to 1.

@wangnengjie
Copy link
Contributor Author

Yes... I remember it is set to 1.

Well, that's... amazing... I debug a while for my failed test. I think I should make another pr to make it works right.

Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
@wangnengjie wangnengjie changed the title [WIP] Feature: Ingest External SST Feature: Ingest External SST Sep 1, 2022
Signed-off-by: wangnengjie <751614701@qq.com>
@wangnengjie
Copy link
Contributor Author

Generally done. You can have a look. @GanZiheng

BTW, the rustfmt.toml use many flags that are only available in nightly channel while CI use stable channel. So it just failed though I run make format under nightly channel.

src/ops/oracle.rs Show resolved Hide resolved
@@ -44,6 +44,7 @@ message ManifestChange {
uint64 key_id = 4;
EncryptionAlgo encryption_algo = 5;
uint32 compression = 6; // Only used for CREATE Op.
uint64 global_version = 7; // Only used for file ingest, 0 means no global_version
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to add global_version just for the ingest external file scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. For ingested files, version of every key is invalid. Or if you have any better idea to assign version for keys in ingested files? Iter on every key and issue random writes is not a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. For ingested files, version of every key is invalid. Or if you have any better idea to assign version for keys in ingested files? Iter on every key and issue random writes is not a good idea.

How about rewriting files to be ingested? In this way, we can also deal with the large value cannot be stored in value log.

Copy link
Contributor Author

@wangnengjie wangnengjie Sep 6, 2022

Choose a reason for hiding this comment

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

Yes. For ingested files, version of every key is invalid. Or if you have any better idea to assign version for keys in ingested files? Iter on every key and issue random writes is not a good idea.

How about rewriting files to be ingested? In this way, we can also deal with the large value cannot be stored in value log.

I think it really costs to rewrite. The bandwidth on cloud device is precious. Large value can store to value log when compactioning ingested files (do we impl this?). And, when rewriting, we are not sure these files can finally success to ingest and so split large values to value log needs more think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we only write value log when writing entry to LSM tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we only write value log when writing entry to LSM tree.

Split during compaction is nice when the threshold changes dynamically or reopen db with different config.

Well, we can discuss this point tomorrow or offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. For ingested files, version of every key is invalid. Or if you have any better idea to assign version for keys in ingested files? Iter on every key and issue random writes is not a good idea.

How about rewriting files to be ingested? In this way, we can also deal with the large value cannot be stored in value log.

I think this will block read process. We need to get commit_ts (version) before rewrite and due to SSI protection, every read_ts after that should wait for this txn to finish (watermark).

src/ingest.rs Show resolved Hide resolved
Bytes::copy_from_slice(user_key(file.table.as_ref().unwrap().smallest())),
Bytes::copy_from_slice(user_key(file.table.as_ref().unwrap().biggest())),
);
let version = self.core.orc.new_ingest_commit_ts(vec![range], &self.opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we only get commit timestamp once for all external files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to. But for files that overlap with each other, there might be same key. And if we get one commit_ts for all these files, there will be same key with same version in different SST which breaks the protection.

@GanZiheng
Copy link
Contributor

Generally done. You can have a look. @GanZiheng

BTW, the rustfmt.toml use many flags that are only available in nightly channel while CI use stable channel. So it just failed though I run make format under nightly channel.

We currently use stable channel for the sake of avoiding CI failing too often. I think there is a format error in this version of code.

@wangnengjie
Copy link
Contributor Author

Generally done. You can have a look. @GanZiheng
BTW, the rustfmt.toml use many flags that are only available in nightly channel while CI use stable channel. So it just failed though I run make format under nightly channel.

We currently use stable channel for the sake of avoiding CI failing too often. I think there is a format error in this version of code.

As what I have said, rustfmt.toml use many flags that are only available in nightly channel which makes the result of format different with stable channel. I will use stable channel to format the code later. So why not just remove all these flags that only available in nightly channel?

Does CI often failed in nightly channel? I saw that all tests (except the first one) run in nightly channel for the support of sanitizer...

@GanZiheng
Copy link
Contributor

GanZiheng commented Sep 6, 2022

Does CI often failed in nightly channel? I saw that all tests (except the first one) run in nightly channel for the support of sanitizer...

Specifically, failed due to the clippy check. We can consider remove these flags or just turn back to nightly channel, I think that's not matter.

Signed-off-by: wangnengjie <751614701@qq.com>
Signed-off-by: wangnengjie <751614701@qq.com>
@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #188 (f241a88) into master (070ff80) will increase coverage by 1.16%.
The diff coverage is 94.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   89.63%   90.80%   +1.16%     
==========================================
  Files          39       40       +1     
  Lines        8838     9679     +841     
==========================================
+ Hits         7922     8789     +867     
+ Misses        916      890      -26     

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.

2 participants