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

[db] add KvVersioned interface #4041

Merged
merged 3 commits into from
Apr 16, 2024
Merged

[db] add KvVersioned interface #4041

merged 3 commits into from
Apr 16, 2024

Conversation

dustinxie
Copy link
Member

@dustinxie dustinxie commented Jan 4, 2024

Description

Currently the staking index DB saves all delegates and candidates at each epoch, the DB size grows very rapidly due to large size of data stored.

This PR introduces a new DB which can save an item at different version (blockchain's height) only when the value changes. In a quick POC testing, this has significantly reduced the DB size from ~80GB to ~4GB

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

Copy link

sonarcloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

7 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 26.92308% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 77.01%. Comparing base (d716bb9) to head (1e46af9).
Report is 29 commits behind head on master.

❗ Current head 1e46af9 differs from pull request most recent head b7e4bf8. Consider uploading reports for the commit b7e4bf8 to get more accurate results

Files Patch % Lines
db/db_versioned.go 0.00% 38 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4041      +/-   ##
==========================================
+ Coverage   76.51%   77.01%   +0.50%     
==========================================
  Files         340      342       +2     
  Lines       29273    29369      +96     
==========================================
+ Hits        22397    22618     +221     
+ Misses       5761     5654     -107     
+ Partials     1115     1097      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}

// SetVersion sets the version which a Get()/Put() wants to access
func (b *BoltDBVersioned) SetVersion(v uint64) KvVersioned {
Copy link
Contributor

Choose a reason for hiding this comment

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

the v means blockHeight in production?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that's correct

}

// CreateVersionedNamespace creates a namespace to store versioned keys
func (b *BoltDBVersioned) CreateVersionedNamespace(ns string, n uint32) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

n what means?

Copy link
Member Author

Choose a reason for hiding this comment

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

n is the size of key in this namespace

BoltDBVersioned struct {
*BoltDB
version uint64 // version for Get/Put()
versioned map[string]int // buckets for versioned keys
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are many keys, will these two maps be very large?

Copy link
Member

Choose a reason for hiding this comment

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

i think the map key should be namespace instead of key, is it right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the map key is the namespace

// 1. "mta" -- regular bucket storing metadata, key is not versioned
// 2. "act" -- versioned namespace, key length = 20
// 3. "stk" -- versioned namespace, key length = 8
KvVersioned interface {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the following interface:

KvVersioned interface{
    Version(ns string, key []byte) (ver uint64, err error)
    WithVersion(ver uint64) KVStore
    CreateNamespace(ns string, keySize uint32) error
}

Simplify the definition of KvVersioned, it does not support operations such as put/get itself, and must specify the version before proceeding. If need an interface that supports both versioned and non-versioned operations, can define another one.

Copy link
Member

@envestcc envestcc Mar 28, 2024

Choose a reason for hiding this comment

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

If this is an interface that supports both versioned and non-versioned operations at the same time, a method IsVersionedNamespaces(ns string) should also be added, so that when the interface is used, it is known whether to call Put() or SetVersion().Put().

Or the Put() is adapted for both versioned and non-versioned:

  • if versioned, it means put to latest version
  • if non-versioned, just put

Copy link
Member Author

Choose a reason for hiding this comment

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

good suggestions, updated PR
yes, Put() is a common interface for both versioned and non-versioned, the code will check inside and process accordingly

@dustinxie dustinxie changed the title [db] add versioned BoltDB and KvVersionedWithBuffer [db] add KvVersioned interface Apr 8, 2024
Comment on lines 63 to 108
// Put writes a <key, value> record
func (b *BoltDBVersioned) Put(ns string, key, value []byte) error {
if !b.IsReady() {
return ErrDBNotStarted
}
// TODO: implement Put
return nil
}

// Get retrieves the most recent version
func (b *BoltDBVersioned) Get(ns string, key []byte) ([]byte, error) {
if !b.IsReady() {
return nil, ErrDBNotStarted
}
// TODO: implement Get
return nil, nil
}

// Delete deletes a record,if key is nil,this will delete the whole bucket
func (b *BoltDBVersioned) Delete(ns string, key []byte) error {
if !b.IsReady() {
return ErrDBNotStarted
}
// TODO: implement Delete
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need Put, Get, Delete the three methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that is the core function of this KvVersioned concept, and will be implemented in later PRs

}

// Delete deletes a record,if key is nil,this will delete the whole bucket
func (b *BoltDBVersioned) Delete(ns string, key []byte) error {
Copy link
Member

Choose a reason for hiding this comment

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

version param missing

// Version returns the key's most recent version
Version(string, []byte) (uint64, error)
// KvStore returns a handler to perform versioned read/write
KvStore() KvVersioned
Copy link
Member

Choose a reason for hiding this comment

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

KvStore(version uint64) KvVersioned

db/db_versioned.go Outdated Show resolved Hide resolved
@Liuhaai
Copy link
Member

Liuhaai commented Apr 12, 2024

pls add more details as context in the description

uint32 keyLen = 2;
}

message KeyMeta {
Copy link
Member

Choose a reason for hiding this comment

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

how is this used

Copy link
Member Author

Choose a reason for hiding this comment

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

will be used in future PR

@Liuhaai
Copy link
Member

Liuhaai commented Apr 15, 2024

From my understand, the idea of pr is to build a mapping for deduplicated data. Changing the implementation of KVStore is more reasonable than changing boltdb itself, where does simple kv storage jobs.

@dustinxie
Copy link
Member Author

From my understand, the idea of pr is to build a mapping for deduplicated data. Changing the implementation of KVStore is more reasonable than changing boltdb itself, where does simple kv storage jobs.

it's not changing boltdb, rather it's using boltdb's underlying API to construct a KVStore that can return multiple versions of value for the same key

Copy link

sonarcloud bot commented Apr 16, 2024

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dustinxie dustinxie merged commit d811c0d into master Apr 16, 2024
3 checks passed
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.

4 participants