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

HaloDB.size and HaloDB.contains method #27

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

Conversation

akhodakivskiy
Copy link

@akhodakivskiy akhodakivskiy commented Feb 22, 2019

Adding two features:

  • check value size associated with a given key without reading entire value from the disk. The size is stored in the memory index.
  • test if a key is stored in the database.

@wangtao724
Copy link
Contributor

@akhodakivskiy Do you still need this feature. If so, please update and rebase

@akhodakivskiy
Copy link
Author

Rebased. Thanks!

@akhodakivskiy akhodakivskiy changed the title HaloDB.size method HaloDB.size and HaloDB.contains method Nov 30, 2019
int size(byte[] key) {
InMemoryIndexMetaData metaData = inMemoryIndex.get(key);
if (metaData == null) {
return 0;

Choose a reason for hiding this comment

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

perhaps this should be -1 since 0 is a valid value size. Then -1 indicates the key is not present, and 0 indicates a value of size 0.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, fixed. Method HaloDB.get also returns 0 for keys that are not stored. I was following the same logic. But I agree that it should return -1 for abscent keys.

Choose a reason for hiding this comment

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

Interesting... HaloDB.get returns a byte[], but HaloDBInternal has another get method that takes a ByteBuffer and reads the value into it (or truncating it if the value is larger than the buffer). This returns the number of bytes copied into the buffer, and 0 when not present.

However, this method is not used anywhere. Its not tested, its not used by the public facing API, it seems to be dead (and untested!) code.

Copy link

@scottcarey scottcarey left a comment

Choose a reason for hiding this comment

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

Oops, I meant to comment, not review.

@@ -40,6 +40,10 @@ public static HaloDB open(String directory, HaloDBOptions opts) throws HaloDBExc
}
}

public int size(byte[] key) {

Choose a reason for hiding this comment

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

Is 'size' the right name? there is already a size method that returns the number of elements. To avoid confusion, we should consider a different name. Perhaps entrySize or valueSize ?

Also, the public API in HaloDB.java should document the expected behavior -- something like Returns the size of the value if present, or -1 if not present.

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.

3 participants