-
Notifications
You must be signed in to change notification settings - Fork 56
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
Remove interfaces #25
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.
Nice work. Some minor comments below.
A few days past and I'm not completely sure why we still need the OakRBuffer
interface, we can just use read only byte buffers. Please document some of the reasoning in this PR thread and/or commit message.
assert writeLock.isHeldByCurrentThread(); | ||
return value.slice(); | ||
} | ||
|
||
public ByteBuffer getImmutableByteBuffer() { | ||
public ByteBuffer getSlicedReadOnlyByteBuffer() { |
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.
public ByteBuffer getSlicedReadOnlyByteBuffer() { | |
public ByteBuffer getReadOnlyByteBuffer() { |
|
||
@Override | ||
public ByteBuffer getByteBuffer() { | ||
public ByteBuffer getSlicedByteBuffer() { |
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.
public ByteBuffer getSlicedByteBuffer() { | |
public ByteBuffer getByteBuffer() { |
@Override | ||
public double getDouble() { | ||
throw new NotImplementedException(); | ||
public ByteOrder order() { |
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.
Is this in use anywhere? why not remove?
@@ -632,11 +631,16 @@ OakRBuffer get(K key) { | |||
if (lookUp == null || lookUp.handle == null) { | |||
return null; | |||
} | |||
lookUp.handle.readLock(); |
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 fit this logic in Handle
(already introduced Handle::transform
) and avoid exposing the lock completely?
@@ -1050,35 +1048,32 @@ public T next() { | |||
if (handle == null) { | |||
return null; | |||
} | |||
handle.readLock.lock(); | |||
handle.readLock(); |
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 think this may also be moved inside Handle
public void uniTestOakRBuffer(){ | ||
|
||
|
||
Function<ByteBuffer, String> deserialize = (byteBuffer) -> { |
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 not extract a proper method out of this?
|
||
|
||
|
||
@Test |
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 use this:
@Test | |
@Test(expected = ConcurrentModificationException.class) |
and remove the try-catch-fail logic below
e163c57
to
bae2337
Compare
Remove redundant OakWBuffer