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:
[email protected]
