[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
apurtell commented on a change in pull request #2681:
URL: https://github.com/apache/hbase/pull/2681#discussion_r527929852
##
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. Methods which no longer override Service
methods (and fail to compile due to Override annotation) are removed. All
methods required by the Service interface not implemented after this removal
are added.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
apurtell commented on a change in pull request #2681:
URL: https://github.com/apache/hbase/pull/2681#discussion_r527932125
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
##
@@ -248,7 +246,7 @@ public void
preCompactSelection(ObserverContext c,
@Override
public void
postCompactSelection(ObserverContext c,
- Store store, ImmutableList 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.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
apurtell commented on a change in pull request #2681:
URL: https://github.com/apache/hbase/pull/2681#discussion_r527929047
##
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. stopAndWait
is replaced with stopAsync() and awaitTerminated()
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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
apurtell commented on a change in pull request #2681: URL: https://github.com/apache/hbase/pull/2681#discussion_r527925161 ## 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 clearFiles(); Review comment: StoreFileManager is a private class so this interface change is fine. Just want to call out that 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. 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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
apurtell commented on a change in pull request #2681: URL: https://github.com/apache/hbase/pull/2681#discussion_r527923873 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java ## @@ -578,7 +576,7 @@ public void call(RegionObserver oserver, ObserverContext 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. 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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
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 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
[GitHub] [hbase] apurtell commented on a change in pull request #2681: HBASE-25308 [branch-1] Consume Guava from hbase-thirdparty hbase-shaded-miscellaneous
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
is OpenJDK 11+7 GA. There are two files affected in two modules.
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
