apurtell commented on a change in pull request #2681: URL: https://github.com/apache/hbase/pull/2681#discussion_r527911332
########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java ########## @@ -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 Review 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 Java 11+7 GA. There are two files affected in two modules. ########## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/util/Bytes.java ########## @@ -129,7 +126,6 @@ // SizeOf which uses java.lang.instrument says 24 bytes. (3 longs?) public static final int ESTIMATED_HEAP_TAX = 16; - @VisibleForTesting Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/BaseFileCleanerDelegate.java ########## @@ -38,7 +37,11 @@ @Override public boolean apply(FileStatus file) { return isFileDeletable(file); - }}); + } + public boolean test(FileStatus file) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileManager.java ########## @@ -72,7 +70,7 @@ void addCompactionResults( * Clears all the files currently in use and returns them. * @return The files previously in use. */ - ImmutableCollection<StoreFile> clearFiles(); Review comment: StoreFileManager is a private class so this interface change is fine. I made the change here for the same reason as other places. We should not be receiving or returning Guava types in our interfaces. This introduces potential compatibility problems. Guava itself evolves and fairly frequently makes breaking changes. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityReplicationEndpoint.java ########## @@ -34,30 +37,29 @@ import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; import org.apache.hadoop.hbase.replication.WALEntryFilter; import org.apache.hadoop.hbase.wal.WAL.Entry; - -import com.google.common.util.concurrent.ListenableFuture; +import org.apache.hbase.thirdparty.com.google.common.util.concurrent.Service; @InterfaceAudience.Private public class VisibilityReplicationEndpoint implements ReplicationEndpoint { Review comment: Our ReplicationEndpoint interface extends Guava's Service interface. The Service interface has evolved. Classes such as this one which derive from ReplicationEndpoint must be fixed up. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityReplicationEndpoint.java ########## @@ -34,30 +37,29 @@ import org.apache.hadoop.hbase.replication.ReplicationPeerConfig; import org.apache.hadoop.hbase.replication.WALEntryFilter; import org.apache.hadoop.hbase.wal.WAL.Entry; - -import com.google.common.util.concurrent.ListenableFuture; +import org.apache.hbase.thirdparty.com.google.common.util.concurrent.Service; @InterfaceAudience.Private public class VisibilityReplicationEndpoint implements ReplicationEndpoint { private static final Log LOG = LogFactory.getLog(VisibilityReplicationEndpoint.class); - private ReplicationEndpoint delegator; + private ReplicationEndpoint delegate; Review comment: Here I am being pedantic about word usage. :-) Since I had to heavily modify this compilation unit anyway. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java ########## @@ -578,7 +576,7 @@ public void call(RegionObserver oserver, ObserverContext<RegionCoprocessorEnviro * @param selected The store files selected to compact * @param request custom compaction */ - public void postCompactSelection(final Store store, final ImmutableList<StoreFile> selected, Review comment: Another downstream consequence of the RegionObserver API fix. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java ########## @@ -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) { } Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java ########## @@ -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) { } @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) { Review comment: Same as above in this unit. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java ########## @@ -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); Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/CompactionRequest.java ########## @@ -295,6 +294,9 @@ public String toString() { public boolean apply(StoreFile sf) { return sf.getReader() != null; } + public boolean test(StoreFile sf) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java ########## @@ -923,12 +921,12 @@ private void bulkLoadHFile(StoreFile sf) throws IOException { } @Override - public ImmutableCollection<StoreFile> close() throws IOException { Review comment: The Store interface specifies a return type of Collection. Guava's ImmutableCollection extends Collection, sure, but unnecessarily binds all (private) users of HStore#close to Guava, and exposes this derived interface to the vagaries of Guava's evolution over time. Fix. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/TableCfWALEntryFilter.java ########## @@ -77,6 +76,9 @@ public boolean apply(byte[] fam) { } return false; } + public boolean test(byte[] fam) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/compaction/MajorCompactor.java ########## @@ -139,6 +140,9 @@ private boolean futuresComplete(List<Future<?>> futures) { @Override public boolean apply(Future<?> input) { return input.isDone(); } + public boolean test(Future<?> input) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java ########## @@ -229,7 +227,7 @@ void postCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, */ @Deprecated void postCompactSelection(final ObserverContext<RegionCoprocessorEnvironment> c, - final Store store, final ImmutableList<StoreFile> selected); Review comment: Same as above in this unit. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ScopeWALEntryFilter.java ########## @@ -58,6 +57,9 @@ public Cell filterCell(Entry entry, Cell cell) { public boolean apply(byte[] fam) { return !scopes.containsKey(fam) || scopes.get(fam) == HConstants.REPLICATION_SCOPE_LOCAL; } + public boolean test(byte[] fam) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationHFileCleaner.java ########## @@ -79,6 +78,9 @@ public boolean apply(FileStatus file) { } return !foundHFileRefInQueue; } + public boolean test(FileStatus file) { Review 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. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestRegionReplicaReplicationEndpointNoMaster.java ########## @@ -253,7 +250,8 @@ public void testRegionReplicaReplicationEndpointReplicate() throws Exception { when(context.getMetrics()).thenReturn(mock(MetricsSource.class)); replicator.init(context); - replicator.start(); + replicator.startAsync(); Review 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() ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/SortedCompactionPolicy.java ########## @@ -202,6 +201,9 @@ public abstract boolean needsCompaction(final Collection<StoreFile> storeFiles, public boolean apply(StoreFile input) { return input.excludeFromMinorCompaction(); } + public boolean test(StoreFile input) { Review 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. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/handler/TestEnableTableHandler.java ########## @@ -138,6 +139,9 @@ public void testEnableTableWithNoRegionServers() throws Exception { public boolean apply(HRegionInfo input) { return input.getTable().equals(tableName); } + public boolean test(HRegionInfo input) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java ########## @@ -85,7 +85,11 @@ public boolean apply(FileStatus file) { } } return !logInReplicationQueue; - }}); + } + public boolean test(FileStatus file) { Review 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. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java ########## @@ -270,7 +269,8 @@ private void uninitialize() { metrics.clear(); if (replicationEndpoint.state() == Service.State.STARTING || replicationEndpoint.state() == Service.State.RUNNING) { - replicationEndpoint.stopAndWait(); + replicationEndpoint.stopAsync(); Review comment: This unit contains several changes required by evolution in Guava's Service interface. Our ReplicationEndpoint extends this interface. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/HBaseInterClusterReplicationEndpoint.java ########## @@ -450,13 +447,6 @@ protected void doStop() { notifyStopped(); } - // is this needed? Nobody else will call doStop() otherwise - @Override - public State stopAndWait() { Review comment: Guava's Service interface has changed. stopAndWait() is replaced with stopAsync() and awaitTerminated(). In any case, I reviewed Guava code and doStop is in fact called when the service is transitioned out of RUNNING state by stopAsync. Something like this override is not needed. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java ########## @@ -208,7 +208,8 @@ protected void doStop() { // completes } }; - replicationEndpoint.start(); + replicationEndpoint.startAsync(); Review comment: Our ReplicationEndpoint interface extends Guava's Service interface. The Service interface has evolved. start() is replaced by startAsync() and awaitRunning() ########## File path: pom.xml ########## @@ -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> Review 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.) ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java ########## @@ -248,7 +246,7 @@ public void preCompactSelection(ObserverContext<RegionCoprocessorEnvironment> c, @Override public void postCompactSelection(ObserverContext<RegionCoprocessorEnvironment> c, - Store store, ImmutableList<StoreFile> selected) { Review comment: This is another downstream consequence of the RegionObserver API fix. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org