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

Add PebbleDB (WIP) #2145

Draft
wants to merge 9 commits into
base: dev
Choose a base branch
from
Draft

Add PebbleDB (WIP) #2145

wants to merge 9 commits into from

Conversation

junha-ahn
Copy link

@junha-ahn junha-ahn commented May 5, 2024

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

CLA assistant check
All committers have signed the CLA.

@hyeonLewis hyeonLewis marked this pull request as draft May 5, 2024 10:27
@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

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

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

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.

@junha-ahn
Copy link
Author

junha-ahn commented May 14, 2024

Metics

I asked about metics 4 days ago. Note it please

Lint

Ok, I followed by LIINT GUIDE executed below commands:

klaytn$ gofumpt -w .
klaytn$ goimports -w .

When I run make lint on my local machine, it prints a lot of lint-bug that I didn't touch. So I don't know how to pass lint-ci

$ make lint 

env GOPATH=/Users/junha/go GO111MODULE=on go run build/ci.go lint
>>> /Users/junha/go/bin/golangci-lint run --tests --disable-all --timeout=10m --presets=format --presets=performance ./...
metrics/librato/client.go:94:38: response body must be closed (bodyclose)
        if resp, err = http.DefaultClient.Do(req); err != nil {
                                            ^
metrics/librato/client.go:87:31: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
        if req, err = http.NewRequest("POST", MetricsPostUrl, bytes.NewBuffer(js)); err != nil {
                                     ^
console/jsre/pretty.go:32: File is not `gci`-ed with --skip-generated -s standard,default (gci)

consensus/istanbul/validator.go:26: File is not `gci`-ed with --skip-generated -s standard,default (gci)
        "github.com/klaytn/klaytn/params"

...

Test Failed on my local machine

image

Run by vscode go-extentions

When I run cmd/utils/nodecmd/flags_test.go on my local machine, it fails every time. Do you know anything about it?

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

4 participants