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

Adding getDbSize functionality #135

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

Conversation

manishankarbalu
Copy link

No description provided.

@@ -285,6 +285,10 @@ func (bdb *badgerDB) CompareAndSet(key, expect, update []byte) (bool, error) {
return err == nil, err
}

func (bdb *badgerDB) GetKeySpaceSize() (int64, error) {
return 0, nil
Copy link
Author

Choose a reason for hiding this comment

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

@@ -26,6 +26,9 @@ service DKV {
// CompareAndSet offers the standard CAS style transaction over a given
// key. Intended to be used in concurrent workloads with less contention.
rpc CompareAndSet (CompareAndSetRequest) returns (CompareAndSetResponse);

//GetKeySpaceSize returns the estimated number of keys the db
rpc GetKeySpaceSize (google.protobuf.Empty) returns (KeySpaceSizeResponse);
Copy link
Author

Choose a reason for hiding this comment

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

Can be renamed to GetKeySpaceCount to avoid ambiguity between size and count, as most lsm based stores uses term size to denote mem/disk used in bytes

Copy link
Member

Choose a reason for hiding this comment

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

IMO we should have consistency in name of the exposed command and the rpc method name.

* This methods returns the approximate count of keys in the DKV database
* @return retunrn the count of keys
*/
long getDbSize();
Copy link
Member

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 add get as a prefix as its implicit that dBSize is a read-only operation

@@ -32,6 +32,7 @@ var cmds = []*cmd{
{"removeNode", "<nexusUrl>", "Remove a master node from DKV cluster", (*cmd).removeNode, "", false},
{"listNodes", "", "Lists the various DKV nodes that are part of the Nexus cluster", (*cmd).listNodes, "", true},
{"getClusterInfo", "<dcId> <database> <vBucket>", "Gets the latest cluster info", (*cmd).getStatus, "", true},
{"getDbSize", "", "Fetches the approximate count of keys in the db", (*cmd).getDbSize, "", true},
Copy link
Member

Choose a reason for hiding this comment

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

Same here, let's name this as dbsize to simplify.

Copy link
Member

Choose a reason for hiding this comment

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

Also why is it approximate?

@@ -42,6 +42,8 @@ type KVStore interface {
// If the expected value is `nil`, then the key is created and
// initialized with the given value, atomically.
CompareAndSet(key, expect, update []byte) (bool, error)
//GetKeySpaceSize returns the estimated number of keys the db
Copy link
Member

Choose a reason for hiding this comment

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

Why is this estimated?

@@ -205,6 +205,10 @@ func (ms *memStore) CompareAndSet(key, expect, update []byte) (bool, error) {
return true, nil
}

func (ms *memStore) GetKeySpaceSize() (int64, error) {
return 0, nil
Copy link
Member

Choose a reason for hiding this comment

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

This could be the count of keys in the db.

// Status indicates the result of the Get operation
Status status = 1;
//Estimated number of keys in the db
int64 dbSize = 2;
Copy link
Member

Choose a reason for hiding this comment

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

could be simply size or count

@@ -380,6 +381,17 @@ func (rdb *rocksDB) CompareAndSet(key, expect, update []byte) (bool, error) {
return err == nil, err
}

func (rdb *rocksDB) GetKeySpaceSize() (int64, error) {
defer rdb.opts.statsCli.Timing("rocksdb.property.latency.dbSize", time.Now())
Copy link
Member

Choose a reason for hiding this comment

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

The metric name doesn't match function name. Let's be consistent.

Status status = 1;
//Estimated number of keys in the db
int64 dbSize = 2;
}
Copy link
Member

Choose a reason for hiding this comment

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

Empty line at end of file missing.

@@ -262,6 +263,13 @@ func (dkvClnt *DKVClient) Iterate(keyPrefix, startKey []byte) (<-chan *KVPair, e
return ch, nil
}

// GetDbSize returns the approximate count of the number of the keys in the db
func (dkvClnt *DKVClient) GetDbSize() (*serverpb.KeySpaceSizeResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Could be better named as DBSize

@@ -260,6 +260,12 @@ public void delete(byte[] key) {
return dkvClient.iterate(startKey, keyPref);
}

@Override
public long getDbSize() {
//TODO: implement this behaviour
Copy link
Member

Choose a reason for hiding this comment

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

Probably this shouldn't even be exposed to java client unless its an exact value.

@@ -26,6 +26,9 @@ service DKV {
// CompareAndSet offers the standard CAS style transaction over a given
// key. Intended to be used in concurrent workloads with less contention.
rpc CompareAndSet (CompareAndSetRequest) returns (CompareAndSetResponse);

//GetKeySpaceSize returns the estimated number of keys the db
rpc GetKeySpaceSize (google.protobuf.Empty) returns (KeySpaceSizeResponse);
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should have consistency in name of the exposed command and the rpc method name.

@kingster kingster linked an issue Mar 23, 2022 that may be closed by this pull request
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.

Implement dbsize as a command
3 participants