DeletedMemoryAccessException is Exception#210
Conversation
sanastas
left a comment
sanastas
left a comment
There was a problem hiding this comment.
I will continue, but I think we need a meeting.
| 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.
The comment above speaks about orderedChunk, do we know it is not going to happen for hash?
There was a problem hiding this comment.
Can you please add an explanatory comment here?
| try { | ||
| chunk.lookUp(ctx, key); | ||
| } catch (DeletedMemoryAccessException e) { | ||
| continue; |
There was a problem hiding this comment.
Can we add an assert that chunk is not hashChunk? As a sanity check...
There was a problem hiding this comment.
this opened to be discussed
| } | ||
| try { | ||
| chunkIter = getChunkIter(chunk); | ||
| } catch (DeletedMemoryAccessException e) { |
There was a problem hiding this comment.
Please add a comment when getChunkIter() can return DeletedMemoryAccessException e
| long offHeapHeader = getOffHeapHeader(headerAddress); | ||
| if (RC.isReferenceDeleted(offHeapHeader)) { | ||
| throw new DeletedMemoryAccessException(); | ||
| return ValueUtils.ValueResult.FALSE; |
There was a problem hiding this comment.
What is the contract of the method? Should be clearly defined. Let'd discuss whether to return false or to throw exception.
| * @throws DeletedMemoryAccessException | ||
| */ | ||
| 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.
Why exception here, upon entryOrderedSet.readKey() failure?
sanastas
left a comment
sanastas
left a comment
There was a problem hiding this comment.
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.
| 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.
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
nextFreeIndexis concurrently updated this is a bug... Maybe to add an explanatory comment here and simplify the loop condition. I think checking onlynextFreeIndexshould be enough.
| 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.
Can you please add an explanatory comment here?
| 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.
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.
2 participants
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.