HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous #2681
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
This project produced a massive PR that touches ~535 files. I will go through this PR tomorrow and place line comments where significant changes to interfaces are made to highlight where review would be helpful. |
|
What the version of the hbase-thirdparty is used at branch-1, AFAIK the newest shaded guava in hbase-thirdparty does not support JDK7, which is not suitable for branch-1? |
|
That's correct. The guava in hbase third-party is currently one that requires jdk8. However we could switch from the guava distro labeled "jre" to the one labeled "android". AFAIK it is the same version but the implementation works for jdk7 still. |
|
for context, see the overview of android flavors which calls out the android flavor for those who need jdk7:
We use release 30, that still has both. Looks like there are some differences; at a glance looks related to jdk8 specific stuff: jdiff report for 30-android vs 30-jre. |
Looking at the diffs, I suggest that we make a new release line of hbase-thirdparty for 1.x. We have deeply involved with JDK8 features on 2.x, and Guava has been widely used across the whole code base so it will be a pain if we can not use JDK8 specific feature on 2.x. It is not the same with the gson library. What do you think @apurtell ? |
|
Ugh. @Apache9 thank you for pointing this out and thank you @busbey for confirming, I didn't even think of potential Java 7 compat problems. :-( (Would have found out the hard way at next RC time trying to build with Java 7...)
I like this proposal:
If you all don't mind the extra effort, when that is done, this PR could be revisited. |
|
I will still go through this PR and leave my own review pointing out the compatibility breaks due to datatype changes. I propose we make a clean break away from Guava types (should never have made them part of our API) but based on your feedback, if any, could make compatibility methods -- I think specifically of changes in Service that impact replication. The compatibility breaks, if accepted, should be forward ported for consistency, so please consider that too when making a review. |
| @@ -436,7 +434,6 @@ public void setOperationTimeout(int operationTimeout) { | |||
| * @return pool if non null, otherwise returns this.pool if non null, otherwise throws | |||
| * RuntimeException | |||
| */ | |||
| @VisibleForTesting | |||
There was a problem hiding this comment.
There is some kind of compiler problem with this particular annotation and this particular unit. The annotation is available on the classpath and other kinds of references (classes, methods) from the same shaded package are fine.
I would argue that overall our usage of VisibleForTesting brings a binding to Guava for no good reason. We could make our own annotation that communicates the same things. However, I do not propose removal or replacement of VisibleForTesting in this PR (even though it would be a good long term thing), only specifically where it is causing odd compiler problems. The compiler used is OpenJDK 11+7 GA. There are two files affected in two modules.
| @@ -129,7 +126,6 @@ | |||
| // SizeOf which uses java.lang.instrument says 24 bytes. (3 longs?) | |||
| public static final int ESTIMATED_HEAP_TAX = 16; | |||
|
|
|||
| @VisibleForTesting | |||
There was a problem hiding this comment.
There is some kind of compiler problem with this particular annotation and this particular unit. The annotation is available on the classpath and other kinds of references (classes, methods) from the same shaded package are fine.
I would argue that overall our usage of VisibleForTesting brings a binding to Guava for no good reason. We could make our own annotation that communicates the same things. However, I do not propose removal or replacement of VisibleForTesting in this PR (even though it would be a good long term thing), only specifically where it is causing odd compiler problems. The compiler used is OpenJDK 11+7 GA. There are two files affected in two modules.
| @@ -180,11 +178,11 @@ public void preCompactSelection(final ObserverContext<RegionCoprocessorEnvironme | |||
|
|
|||
| @Override | |||
| public void postCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, | |||
| final Store store, final ImmutableList<StoreFile> selected) { } | |||
| final Store store, final List<StoreFile> selected) { } | |||
There was a problem hiding this comment.
Downstream consequence of a RegionObserver API fix. This method should not receive Guava's ImmutableList as type, it should simply be List. Using ImmutableList to represent the fact that this list is, and should be, immutable, is nice, but binds a LimitedPrivate interface to the vagaries of Guava's frequent breaking changes as it itself evolves.
|
|
||
| @Override | ||
| public void postCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, | ||
| final Store store, final ImmutableList<StoreFile> selected, CompactionRequest request) { | ||
| final Store store, final List<StoreFile> selected, CompactionRequest request) { |
There was a problem hiding this comment.
Same as above in this unit.
| @@ -216,7 +214,7 @@ void preCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, | |||
| * @param request custom compaction request | |||
| */ | |||
| void postCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, | |||
| final Store store, final ImmutableList<StoreFile> selected, CompactionRequest request); | |||
| final Store store, final List<StoreFile> selected, CompactionRequest request); | |||
There was a problem hiding this comment.
RegionObserver API fix. This method should not receive Guava's ImmutableList as type, it should simply be List. Using ImmutableList to represent the fact that this list is, and should be, immutable, is nice, but binds a LimitedPrivate interface to the vagaries of Guava's frequent breaking changes as it itself evolves.
| @@ -248,7 +246,7 @@ public void preCompactSelection(ObserverContext<RegionCoprocessorEnvironment> c, | |||
|
|
|||
| @Override | |||
| public void postCompactSelection(ObserverContext<RegionCoprocessorEnvironment> c, | |||
| Store store, ImmutableList<StoreFile> selected) { | |||
There was a problem hiding this comment.
Downstream consequence of a RegionObserver API fix. This method should not receive Guava's ImmutableList as type, it should simply be List. Using ImmutableList to represent the fact that this list is, and should be, immutable, is nice, but binds a LimitedPrivate interface to the vagaries of Guava's frequent breaking changes as it itself evolves.
| @@ -138,6 +139,9 @@ public void testEnableTableWithNoRegionServers() throws Exception { | |||
| public boolean apply(HRegionInfo input) { | |||
| return input.getTable().equals(tableName); | |||
| } | |||
| public boolean test(HRegionInfo input) { | |||
There was a problem hiding this comment.
Every use of Predicate gets this update. It is annoying. Guava's predicate extends Java's Predicate. At some point between Java 7 and Java 11 (which I am using as primary compiler these days) this method 'test' is now required by the Java Predicate interface, even if it is only apply() that is the interesting method that will be called. To preserve compatibility with earlier versions no Override annotation is placed here.
Perhaps long term we should migrate away from use of Guava's Predicate.
I will include this comment once per compilation unit where this change was necessary.
| @@ -208,7 +208,8 @@ protected void doStop() { | |||
| // completes | |||
| } | |||
| }; | |||
| replicationEndpoint.start(); | |||
| replicationEndpoint.startAsync(); | |||
There was a problem hiding this comment.
Our ReplicationEndpoint interface extends Guava's Service interface. The Service interface has evolved. start() is replaced by startAsync() and awaitRunning()
| @@ -253,7 +250,8 @@ public void testRegionReplicaReplicationEndpointReplicate() throws Exception { | |||
| when(context.getMetrics()).thenReturn(mock(MetricsSource.class)); | |||
|
|
|||
| replicator.init(context); | |||
| replicator.start(); | |||
| replicator.startAsync(); | |||
There was a problem hiding this comment.
Our ReplicationEndpoint interface extends Guava's Service interface. The Service interface has evolved. start() has been replaced by startAsync(). startAndWait has been replaced with startAsync() and awaitRunning(). stop() has been replaced by stopAsync(). stopAndWait has been replaced with stopAsync() and awaitTerminated()
| @@ -1314,6 +1313,7 @@ | |||
| <bouncycastle.version>1.46</bouncycastle.version> | |||
| <kerby.version>1.0.1</kerby.version> | |||
| <hbase.shaded.gson.version>3.0.0</hbase.shaded.gson.version> | |||
| <hbase.shaded.miscellaneous.version>3.0.0</hbase.shaded.miscellaneous.version> | |||
There was a problem hiding this comment.
The latest is 3.4.0.
I just copied the version supplied for hbase-shaded-gson here.
Both probably should be updated.
Based on discussion elsewhere on the PR, currently all released versions of hbase-shaded-miscellaneous are unsuitable for branch-1 because the shaded Guava's class files require Java 8 or above to execute. Presumably a new release of hbase-shaded artifacts suitable for Java 7 can be done and this will change to reflect that version. (And there might be an artifact name change, or some trick with classifiers.)
|
I filed HBASE-25316 for a hbase-thirdparty release with a hbase-shaded-miscellaneous (guava) suitable for Java 7 and branch-1 |
|
HBASE-24640 proposes to eliminate VisibleForTesting, which will make this patch much smaller. I am working on HBASE-24640 now and will update this PR once the removal has been merged. |
apurtell
force-pushed
the
HBASE-25308-branch-1
branch
from
cfaf0fe
to
ac52b06
Compare
|
Rebased and updated after HBASE-24640 |
|
💔 -1 overall
This message was automatically generated. |
apurtell
force-pushed
the
HBASE-25308-branch-1
branch
from
ac52b06
to
e03225b
Compare
|
💔 -1 overall
This message was automatically generated. |
I propose to do a global search and replace of all direct uses of Guava in our branch-1 code base and refer to Guava as provided in hbase-thirdparty's hbase-shaded-miscellaneous. This will protect HBase branch-1 from Guava cross-version vagaries just like the same technique protects branch-2 and branch-2 based releases.
There are a couple of Public or LimitedPrivate interfaces that incorporate Guava's HostAndPort and Service that will be indirectly impacted. We are about to release a new minor branch-1 version, 1.7.0, and this would be a great opportunity to introduce this kind of change in a manner consistent with semantic versioning and our compatibility policies.
99% of the changes are string replacements on imports, and reordering of imports to head off checkstyle warnings. There will still be a ton of checkstyle nits I'm sure.