-
Notifications
You must be signed in to change notification settings - Fork 46
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
DeletedMemoryAccessException is Exception #210
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue, but I think we need a meeting.
@@ -166,7 +166,11 @@ V replace(K key, V value, OakTransformer<V> valueDeserializeTransformer) { | |||
|
|||
for (int i = 0; i < MAX_RETRIES; i++) { | |||
BasicChunk<K, V> chunk = findChunk(key, ctx); // find orderedChunk matching key | |||
chunk.lookUp(ctx, key); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above speaks about orderedChunk, do we know it is not going to happen for hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add an explanatory comment here?
try { | ||
chunk.lookUp(ctx, key); | ||
} catch (DeletedMemoryAccessException e) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an assert that chunk is not hashChunk? As a sanity check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this opened to be discussed
} | ||
try { | ||
chunkIter = getChunkIter(chunk); | ||
} catch (DeletedMemoryAccessException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment when getChunkIter()
can return DeletedMemoryAccessException e
@@ -50,12 +50,13 @@ private static boolean atomicallySetDeleted(long headerAddress, long offHeapMeta | |||
ValueUtils.ValueResult preRead(final int onHeapVersion, long headerAddress) { | |||
long offHeapHeader = getOffHeapHeader(headerAddress); | |||
if (RC.isReferenceDeleted(offHeapHeader)) { | |||
throw new DeletedMemoryAccessException(); | |||
return ValueUtils.ValueResult.FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the contract of the method? Should be clearly defined. Let'd discuss whether to return false or to throw exception.
*/ | ||
int compareKeyAndEntryIndex(KeyBuffer tempKeyBuff, K key, int ei) { | ||
int compareKeyAndEntryIndex(KeyBuffer tempKeyBuff, K key, int ei) throws DeletedMemoryAccessException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exception here, upon entryOrderedSet.readKey()
failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked over majority of the code. I didn't find something drastic :) Very good job! But we still have some things to discuss. Please go over my comments and also the old ones which still remained unresolved.
@@ -299,7 +299,9 @@ boolean isEntrySetValidAfterRebalance() { | |||
|
|||
void releaseAllDeletedKeys() { | |||
KeyBuffer key = new KeyBuffer(config.keysMemoryManager.getEmptySlice()); | |||
for (int i = 0; i < numOfEntries.get() ; i++) { | |||
for (int i = 0; i < nextFreeIndex.get() - 1 && i < numOfEntries.get() ; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me very weird condition and comment.
releaseAllDeletedKeys()
should be called only by one thread that succeeds to change the chunk's state from FROZEN to RELEASED (CAS success). I do not see any reason for other threads to invokereleaseAllDeletedKeys()
. Can you please check that this is how it happens? And definitely add a header comment aboutreleaseAllDeletedKeys()
usage intention.- This release happens as a very last step in the Chunk rebalance. At the beginning of the rebalance step the thread proceeding with the rebalance waits for all updates to finish. This is why we have publish/unpublish staff. So if
nextFreeIndex
is concurrently updated this is a bug... Maybe to add an explanatory comment here and simplify the loop condition. I think checking onlynextFreeIndex
should be enough.
@@ -166,7 +166,11 @@ V replace(K key, V value, OakTransformer<V> valueDeserializeTransformer) { | |||
|
|||
for (int i = 0; i < MAX_RETRIES; i++) { | |||
BasicChunk<K, V> chunk = findChunk(key, ctx); // find orderedChunk matching key | |||
chunk.lookUp(ctx, key); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add an explanatory comment here?
@@ -471,7 +456,8 @@ protected void invalidatePrevState() { | |||
|
|||
protected abstract BasicChunk<K, V> getNextChunk(BasicChunk<K, V> current); | |||
|
|||
protected abstract BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current); | |||
protected abstract BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to explain in which cases the exception is thrown. As I remember we were asking ourselves as well. Please add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be discussed
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.