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

Another novaentry #200

Merged
merged 17 commits into from
Mar 21, 2022
Merged

Another novaentry #200

merged 17 commits into from
Mar 21, 2022

Conversation

li0nr
Copy link
Collaborator

@li0nr li0nr commented Jan 17, 2022

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Contributor

@sanastas sanastas left a comment

Choose a reason for hiding this comment

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

This is not yet final review, but already quite big, so Ramy can start going over my comments. The code is good.

core/src/main/java/com/yahoo/oak/EntryOrderedSet.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/EntryOrderedSet.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/EntryOrderedSet.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/NovaMemoryManager.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/NovaMemoryManager.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/OrderedChunk.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/ReferenceCodecNova.java Outdated Show resolved Hide resolved
Copy link
Contributor

@sanastas sanastas left a comment

Choose a reason for hiding this comment

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

Now I reviewed the code till the end. Looks good. We still have some items to discuss.

core/src/main/java/com/yahoo/oak/InternalOakMap.java Outdated Show resolved Hide resolved
if (!chunk.readKeyFromEntryIndex(ctx.tempKey, nextIndex)) {
continue;
}
if (!inBounds(ctx.tempKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

inBounds() is invoking tooHigh and tooLow methods, where key comparison happens. First it sounds like here we can get the exception as well. Second, is the comparison in the tooHigh and tooLow methods using KeyUtils?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I totally agree, this will be resolved in the next PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will be fixed when changing the RuntimeException to Exception

core/src/main/java/com/yahoo/oak/NovaMMHeader.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/NovaMMHeader.java Outdated Show resolved Hide resolved
@@ -0,0 +1,277 @@
/*
* Copyright 2020, Verizon Media.
Copy link
Contributor

Choose a reason for hiding this comment

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

I will continue review from here...

Copy link
Contributor

@liran-funaro liran-funaro left a comment

Choose a reason for hiding this comment

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

Initial review

if (!isValueRefValidAndNotDeleted(currIndex)) {
return false;
}
if (isKeyDeleted(new KeyBuffer(config.keysMemoryManager.getEmptySlice()), currIndex)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the ThreadContext here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried something else!

core/src/main/java/com/yahoo/oak/EntryOrderedSet.java Outdated Show resolved Hide resolved
@@ -225,7 +235,7 @@ private int getLastSortedEntryIndex(int sortedCount) {
int compareKeyAndEntryIndex(KeyBuffer tempKeyBuff, K key, int ei) {
boolean isAllocated = entryOrderedSet.readKey(tempKeyBuff, ei);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what we can do here ?
there is a situation where isAllocated will be false.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly what needs to be compared via KeyUtils meaning providing the comparison as lambda to the memory manager (Nova). This method can be used only on minKey. Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am referring to the assert that comes directly after this line
assert isAllocated; do we want to remove it?

@sanastas sanastas merged commit 68485b0 into yahoo:master Mar 21, 2022
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