8000 Add PebbleDB by junha-ahn · Pull Request #2145 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

Add PebbleDB #2145

Closed
wants to merge 11 commits into from
Closed

Add PebbleDB #2145

wants to merge 11 commits into from

Conversation

junha-ahn
Copy link
@junha-ahn junha-ahn commented May 5, 2024 •
edited
Loading

Proposed changes

  • Add PebbleDB

Types of changes

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

...

Further comments

@CLAassistant
Copy link
CLAassistant commented May 5, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@hyeonLewis hyeonLewis marked this pull request as draft May 5, 2024 10:27
blukat29
blukat29 reviewed May 7, 2024
View reviewed changes
@blukat29
Copy link
Contributor
blukat29 commented May 7, 2024

Please include the pebbleDB to db_manager_test so that we can continuously test while you push commits to this PR branch.

@junha-ahn
Copy link
Author
junha-ahn commented May 7, 2024 •
edited
Loading

I added PebbleDB unit test into db_manager_test.go. and the tests I changed run successfully on my local machine. However, Im not sure why the CI jobs failed. (with below log) Do I need to do something?

Error response from daemon: pull access denied for wurstmeister/zookeeper, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

I will also add metrics features soon, and apply lint

and If there are any tests that we need to add, plz let me know, I will try to implement them.

@hyeonLewis
Copy link
Contributor

I added PebbleDB unit test into db_manager_test.go. and the tests I changed run successfully on my local machine. However, Im not sure why the CI jobs failed. (with below log) Do I need to do something?

Error response from daemon: pull access denied for wurstmeister/zookeeper, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

There's an issue related to zookeeper, and will be fixed soon (see #2147)

@junha-ahn
Copy link
Author
junha-ahn commented May 10, 2024 •
edited
Loading

I added metrics and all the logic from pebble.go in Geth

Do I need to make the metrics key the same as in the leveldb_database.go code?

- d.compWriteMeter = metrics.GetOrRegisterMeter(prefix+"compact/output", nil)
+ db.compWriteMeter = metrics.GetOrRegisterMeter(prefix+"compaction/write", nil)

"compact/output" from Geth PebbleDB file
"compaction/write" from Klaytn levelDB file

@junha-ahn
Copy link
Author

NOTE: I need to refer to the RocksDB PR

hyeonLewis
hyeonLewis reviewed May 13, 2024
View reviewed changes
Copy link
Contributor
@hyeonLewis hyeonLewis left a comment

Choose a reason for hiding this comment

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

Did you intentionally remove all comments? If not, please bring comments together.

@junha-ahn
Copy link
Author

Ok, I will bring all the comments.

@hyeonLewis
Copy link
Contributor

I added metrics and all the logic from pebble.go in Geth

Do I need to make the metrics key the same as in the leveldb_database.go code?

They use same key format for levelDB and pebbleDB. So I think it's also better to use same key format on our side. How do you think @blukat29 ?

@blukat29
Copy link
Contributor

@hyeonLewis @junha-ahn Sure, you can use the same metric keys for LevelDB and PebbleDB.

@junha-ahn junha-ahn changed the title Add PebbleDB (WIP) Add PebbleDB Jun 24, 2024
@blukat29
Copy link
Contributor
blukat29 commented Jul 1, 2024

Hello @junha-ahn, would you reopen the pull request to https://github.com/kaiachain/kaia repository? we're migrating to kaiachain repos.

@junha-ahn
Copy link
Author

@blukat29 Ok, I will move PR to Kaia by the end of this week. thank you.

@junha-ahn junha-ahn mentioned this pull request Jul 7, 2024
Merged
9 tasks
@junha-ahn
Copy link
Author

Ok Here is the new PR: kaiachain/kaia#42

@blukat29 blukat29 closed this Jul 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@blukat29 blukat29 blukat29 left review comments

@hyeonLewis hyeonLewis hyeonLewis left review comments

@aidan-kwon aidan-kwon Awaiting requested review from aidan-kwon aidan-kwon is a code owner

@jeongkyun-oh jeongkyun-oh Awaiting requested review from jeongkyun-oh jeongkyun-oh is a code owner

@yoomee1313 yoomee1313 Awaiting requested review from yoomee1313 yoomee1313 is a code owner

@ian0371 ian0371 Awaiting requested review from ian0371 ian0371 will be requested when the pull request is marked ready for review ian0371 is a code owner

@hyunsooda hyunsooda Awaiting requested review from hyunsooda hyunsooda will be requested when the pull request is marked ready for review hyunsooda is a code owner

@2dvorak 2dvorak Awaiting requested review from 2dvorak 2dvorak will be requested when the pull request is marked ready for review 2dvorak is a code owner

@JayChoi1736 JayChoi1736 Awaiting requested review from JayChoi1736 JayChoi1736 will be requested when the pull request is marked ready for review JayChoi1736 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@junha-ahn @CLAassistant @blukat29 @hyeonLewis
0