HBASE-26773 [hbase-thirdparty] Introduce a hbase-unsafe module in hba…#78
HBASE-26773 [hbase-thirdparty] Introduce a hbase-unsafe module in hba…#78Apache9 merged 2 commits intoapache:masterfrom
Conversation
…se-thirdparty to remove the direct references of Unsafe in our main code base
|
💔 -1 overall
This message was automatically generated. |
|
The javac warnings are expected. We do use sun.misc.Unsafe in the package... |
apurtell
left a comment
There was a problem hiding this comment.
One naming nit request, otherwise lgtm
| * Delegate all methods of {@link HBaseUnsafe0}, so we will not touch the actual | ||
| * {@code sun.misc.Unsafe} class until we actually call the methods. | ||
| */ | ||
| public final class HBaseUnsafe { |
There was a problem hiding this comment.
Want to call this PlatformDependent :-) ?
There was a problem hiding this comment.
This is a bit of a joke, but would be fine too for real...
There was a problem hiding this comment.
I'm just fine with either name :)
| * Delegate all the method in sun.misc.Unsafe. | ||
| */ | ||
| @SuppressWarnings("restriction") | ||
| final class HBaseUnsafe0 { |
There was a problem hiding this comment.
This naming is unusual for our code base. We usually do things like HBaseUnsafe -> HBaseUnsafeImpl, or HBaseUnsafe -> HBaseUnsafeInternal. The '0' suffix is efficient but the more typical constructions are better for comprehension.
There was a problem hiding this comment.
I prefer the internal one.
Let me update the patch.
|
💔 -1 overall
This message was automatically generated. |
| private static final boolean UNALIGNED; | ||
|
|
||
| static { | ||
| AVAIL = AccessController.doPrivileged(new PrivilegedAction<Boolean>() { |
There was a problem hiding this comment.
nit: implement as a lambda instead?
There was a problem hiding this comment.
It will cause compile error, as there is a PrivilegedExceptionAction...
| Method m; | ||
| try { | ||
| m = clazz.getDeclaredMethod("arrayBaseOffset", Class.class); | ||
| if (m == null) { |
There was a problem hiding this comment.
Static analysis says that Class#getDeclaredMethod never returns null, so none of these null checks are ever executed. I don't know how descriptive the system-generated error messages will be, but the nice helpful warn statements won't happen.
There was a problem hiding this comment.
I just copied this piece of code from the hbase main repo, without any changes. Maybe it is used to be compatible with some strange JDK implementations?
There was a problem hiding this comment.
Hmm good point, this class implementation could be JDK dependent.
There was a problem hiding this comment.
This is the implementation of the 'Class', not Unsafe, so typically I agree with you that we should get a NoSuchMethodException. Anyway, not a big deal? Can open a new issue to remove these null checks...
There was a problem hiding this comment.
Agreed it's not a big deal. It might be confusing in the future for someone reading code without the aid of a development environment.
…se-thirdparty to remove the direct references of Unsafe in our main code base