yahoo / Oak

A Scalable Concurrent Key-Value Map for Big Data Analytics
Apache License 2.0
268 stars 48 forks source link

Improve doc and implementation of OakUnsafeDirectBuffer.getByteBuffer #209

Closed dynaxis closed 2 years ago

dynaxis commented 2 years ago

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.


This PR improves doc comment on OakUnsafeDirectBuffer.getByteBuffer to better reflect the current implementation after removing internal uses of ByteBuffers.

Also it reimplements proper protection of read-only Oak buffers by returning read only view of ByteBuffer if source buffer implementation is read only. Such a measure was mistakenly removed in the course of the recent elimination of all ByteBuffer usage.

dynaxis commented 2 years ago

Makes sense. I'll update the current PR soon.

dynaxis commented 2 years ago

I added tests for all the implementations of OakUnsafeDirectBuffer to test writability of the returned ByteBuffer. And found that UnscopeBuffer and its subclass violate the spec. I fixed it too in this PR.

Checking if ByteBuffer is read-only is done using ByteBuffer.isReadOnly() different from your sketch above. Since ByteBuffer can't be subclassed, it's safe to rely on it. We don't need to try writing to a ByteBuffer and see if an exception is thrown.

SliceUtils is in com.yahoo.oak package instead of the test_utils subpackage, since some of the classes used by it are not accessible from the subpackage.

In SliceUtils, I didn't bother to release all of the Slices after each test since I close their source MemoryManager. If that's wrong, please let me know.

In the end, the tests are the most complex part of this PR. :)

ADDITIONAL NOTE:

I originally thought that I might obtain OakUnsafeDirectBuffer implementations by constructing an OakMap myself as in InternalOakMapTest, but in such a way, it is difficult to test all of the implementations for obeying the spec. That's why I leaned to an approach with which I can instantiate specific buffer types.

dynaxis commented 2 years ago

I haven't run all of the tests except for those added by me due to some other tests failing in my environment. So do you think the failing test is due to this PR? During my local runs, errors are as follows:

[ERROR] Errors:
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   IteratorModificationTest.init:45->generateString:34 » IllegalFormatArgumentIndex
[ERROR]   NativeMemoryAllocatorTest.allocateContention:83 Mockito
Mockito cannot mock t...
sanastas commented 2 years ago

The failures looks unrelated to your PR and also to Oak in general, as this is just a way of string creation. More than that, all the tests are running upon each of your commits/pushes and you have "All checks have passed" meaning there were no failures on mvn flow.

Is it the first time you run all the tests? Maybe it is related to your local environment, Java version, etc.

sanastas commented 2 years ago

@dynaxis , pleasure to work with you! Your PR looks very good! Thank you! I am merging to master.

dynaxis commented 2 years ago

Happy to work with you and to contribute a small bit to the project I rely on. Thank you.