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

[DNM] import sst reader for rocksdb 5.8 #164

Closed
wants to merge 18 commits into from
Closed

[DNM] import sst reader for rocksdb 5.8 #164

wants to merge 18 commits into from

Conversation

UncP
Copy link

@UncP UncP commented Nov 29, 2017

No description provided.

@UncP UncP changed the title [DNM]import sst reader for rocksdb 5.8 [DNM] import sst reader for rocksdb 5.8 Nov 29, 2017
pub fn crocksdb_sstfilereader_read_table_properties(
reader: *mut SstFileReader,
) -> *mut DBTableProperties;
pub fn crocksdb_sstfilereader_destroy(reader: *mut SstFileReader);

Choose a reason for hiding this comment

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

how can we get key-value?

Copy link
Author

Choose a reason for hiding this comment

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

We don't need to, we can directly get the offset and the size of seq no, change that int the sst file.

Choose a reason for hiding this comment

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

this is a common feature, we should support it.

Copy link
Author

@UncP UncP Nov 30, 2017

Choose a reason for hiding this comment

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

I don't think we can get k-v since rocksdb has no related api for SstFileReader.

Choose a reason for hiding this comment

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

Copy link
Author

@UncP UncP Dec 1, 2017

Choose a reason for hiding this comment

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

I saw that before, this is a callback, we still can't get k-v. The callback is used in SstFileReader, do you mean add this callback api?

Choose a reason for hiding this comment

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

yes

@UncP
Copy link
Author

UncP commented Dec 1, 2017

@huachaohuang PTAL at tests/test_rocksdb_options::test_block_based_options, this test failed.

    assert_ne!(
        opts.get_statistics_ticker_count(TickerType::ReadAmpEstimateUsefulBytes),
        0
    );

left and right are 0, this pr based on rocksdb 5.8.
is this ok that I replace assert_ne with assert_eq?

@huachaohuang
Copy link

@UncP RocksDB 5.8 adds some other statistics enums, which invalidate our TickerType values. This is already fixed in master, you should update your PR.

@UncP
Copy link
Author

UncP commented Dec 7, 2017

this pr has become #173.

@UncP UncP closed this Dec 7, 2017
yiwu-arbug pushed a commit that referenced this pull request Jun 4, 2020
update titan to include tikv/titan#169 for tikv-4.x

include the following changes
```
81814ec 2020-06-04 zbk602423539@gmail.. Fix GC may delete a already deleted blob file (#168) (#169)
80657c0 2020-06-04 zbk602423539@gmail.. Fix wrong assert delta < 0 for cocurrent compaction while flush (#172) (#176)
0db7976 2020-06-04 sre-bot@pingcap.com  Fix wrong live data size when encounter rewrite failure (#161) (#175)
c41f2a2 2020-06-04 sre-bot@pingcap.com  feat: Titan should return user value to compaction filter #163 (#164) (#174)
```

Signed-off-by: sre-bot <sre-bot@pingcap.com>
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.

None yet

3 participants