[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394099926
 
 

 ##
 File path: 
hbase-rest/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
 ##
 @@ -77,6 +73,10 @@
 import org.slf4j.LoggerFactory;
 
 import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
+import org.apache.hbase.thirdparty.com.google.protobuf.Descriptors;
+import org.apache.hbase.thirdparty.com.google.protobuf.Message;
+import org.apache.hbase.thirdparty.com.google.protobuf.Service;
+import org.apache.hbase.thirdparty.com.google.protobuf.ServiceException;
 
 /**
  * HTable interface to remote tables accessed via REST gateway
 
 Review comment:
   This class should be removed?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394099616
 
 

 ##
 File path: hbase-protocol-shaded/src/main/protobuf/AccessControl.proto
 ##
 @@ -128,6 +128,15 @@ message HasUserPermissionsResponse {
 repeated bool has_user_permission = 1;
 }
 
+message HasPermissionRequest {
+  required TablePermission table_permission = 1;
+  required bytes user_name = 2;
+}
 
 Review comment:
   Missing from hbase-protocol-shaded?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r393906696
 
 

 ##
 File path: hbase-client/src/main/java/org/apache/hadoop/hbase/Coprocessor.java
 ##
 @@ -21,11 +21,11 @@
 
 import java.io.IOException;
 import java.util.Collections;
-
-import com.google.protobuf.Service;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.yetus.audience.InterfaceStability;
 
+import org.apache.hbase.thirdparty.com.google.protobuf.Service;
+
 
 Review comment:
   What about CPEPs? Will clients now need access to the hbase-shaded-protobuf 
jar?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394016223
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/client/coprocessor/BigDecimalColumnInterpreter.java
 ##
 @@ -124,7 +125,7 @@ public void initialize(EmptyMsg msg) {
 
   private BigDecimalMsg getProtoForType(BigDecimal t) {
 BigDecimalMsg.Builder builder = BigDecimalMsg.newBuilder();
-return 
builder.setBigdecimalMsg(ByteStringer.wrap(Bytes.toBytes(t))).build();
+return 
builder.setBigdecimalMsg(UnsafeByteOperations.unsafeWrap(Bytes.toBytes(t))).build();
 
 Review comment:
   Good


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394100184
 
 

 ##
 File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
 ##
 @@ -1985,21 +1985,20 @@ public SecurityCapabilitiesResponse 
getSecurityCapabilities(RpcController contro
 
   /**
* Determines if there is a MasterCoprocessor deployed which implements
-   * {@link 
org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos.AccessControlService.Interface}.
+   * {@link AccessControlService.Interface}.
*/
   boolean hasAccessControlServiceCoprocessor(MasterCoprocessorHost cpHost) {
-return checkCoprocessorWithService(
-cpHost.findCoprocessors(MasterCoprocessor.class), 
AccessControlService.Interface.class);
+return 
checkCoprocessorWithService(cpHost.findCoprocessors(MasterCoprocessor.class),
+  AccessControlService.Interface.class);
   }
 
   /**
* Determines if there is a MasterCoprocessor deployed which implements
-   * {@link 
org.apache.hadoop.hbase.protobuf.generated.VisibilityLabelsProtos.VisibilityLabelsService.Interface}.
+   * {@link VisibilityLabelsService.Interface}.
*/
   boolean hasVisibilityLabelsServiceCoprocessor(MasterCoprocessorHost cpHost) {
-return checkCoprocessorWithService(
-cpHost.findCoprocessors(MasterCoprocessor.class),
-VisibilityLabelsService.Interface.class);
+return 
checkCoprocessorWithService(cpHost.findCoprocessors(MasterCoprocessor.class),
+  VisibilityLabelsService.Interface.class);
   }
 
   /**
 
 Review comment:
   What happens when an old CP impl tries to go against hbase3?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394097610
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/CoprocessorRpcUtils.java
 ##
 @@ -87,53 +89,47 @@ public static CoprocessorServiceRequest 
getCoprocessorServiceRequest(
   }
 
   public static CoprocessorServiceRequest getCoprocessorServiceRequest(
-  final Descriptors.MethodDescriptor method, final Message request, final 
byte [] row,
-  final byte [] regionName) {
-return CoprocessorServiceRequest.newBuilder().setCall(
-getCoprocessorServiceCall(method, request, row)).
-  setRegion(RequestConverter.buildRegionSpecifier(REGION_NAME, 
regionName)).build();
+final Descriptors.MethodDescriptor method, final Message request, final 
byte[] row,
+final byte[] regionName) {
+return CoprocessorServiceRequest.newBuilder()
+  .setCall(getCoprocessorServiceCall(method, request, row))
+  .setRegion(RequestConverter.buildRegionSpecifier(REGION_NAME, 
regionName)).build();
   }
 
-  private static CoprocessorServiceCall getCoprocessorServiceCall(
-  final Descriptors.MethodDescriptor method, final Message request, final 
byte [] row) {
+  private static CoprocessorServiceCall getCoprocessorServiceCall(final 
MethodDescriptor method,
+final Message request, final byte[] row) {
 return CoprocessorServiceCall.newBuilder()
-
.setRow(org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations.unsafeWrap(row))
-.setServiceName(CoprocessorRpcUtils.getServiceName(method.getService()))
-.setMethodName(method.getName())
-// TODO! Come back here after! This is a double copy of the 
request if I read
-// it right copying from non-shaded to shaded version!! FIX!
-
.setRequest(org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations.
-unsafeWrap(request.toByteArray())).build();
+  
.setRow(org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations.unsafeWrap(row))
+  .setServiceName(CoprocessorRpcUtils.getServiceName(method.getService()))
+  .setMethodName(method.getName())
+  // TODO! Come back here after! This is a double copy of the 
request if I read
+  // it right copying from non-shaded to shaded version!! FIX!
 
 Review comment:
   Did I write this comment?
   
   In CPEP, there was copying from pb2.5 data structures to internal PB3 data 
structures -- so we could maintain pb2.5 as public interface but this seems to 
be something different.


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394098204
 
 

 ##
 File path: hbase-endpoint/pom.xml
 ##
 @@ -47,24 +47,6 @@
   true
 
   
-  
-org.xolstice.maven.plugins
-protobuf-maven-plugin
-
-  
-compile-protoc
-generate-sources
-
-  compile
-
- 
-  
-
${basedir}/../hbase-protocol/src/main/protobuf
-  
- 
-  
-
-  
 
 Review comment:
   It doesn't have pb local anymore?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394097852
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/security/token/ClientTokenUtil.java
 ##
 @@ -69,7 +70,7 @@ private static void injectFault() throws ServiceException {
   AsyncConnection conn) {
 CompletableFuture> future = new 
CompletableFuture<>();
 if (injectedException != null) {
-  future.completeExceptionally(injectedException);
+  
future.completeExceptionally(ProtobufUtil.handleRemoteException(injectedException));
 
 Review comment:
   Needed?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r393908442
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTable.java
 ##
 @@ -35,6 +34,7 @@
 import org.apache.yetus.audience.InterfaceAudience;
 
 import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
+import org.apache.hbase.thirdparty.com.google.protobuf.RpcChannel;
 
 Review comment:
   Yeah, ditto. These are public classes. This stuff is used internally so 
should be fine just changing it... its just that hbase-shaded-protobuf needs to 
be on the CLASSPATH? I suppose it was anyways?


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r394098972
 
 

 ##
 File path: 
hbase-endpoint/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionServerCoprocessorEndpoint.java
 ##
 @@ -20,22 +20,14 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
-import com.google.protobuf.RpcCallback;
-import com.google.protobuf.RpcController;
-import com.google.protobuf.Service;
 
 Review comment:
   Yeah, so client now has to have hbase-shaded-protobuf on its side and if we 
change internal pb version we could break the endpoint?
   
   Realistically, the pb3 should probably stay on-the-wire compat so should be 
concern till pb4 or until we decided we want our pbs to be schema v3 compat.


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r393907627
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
 ##
 @@ -71,6 +65,13 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import 
org.apache.hbase.thirdparty.com.google.protobuf.Descriptors.MethodDescriptor;
+import org.apache.hbase.thirdparty.com.google.protobuf.Message;
+import org.apache.hbase.thirdparty.com.google.protobuf.RpcCallback;
+import org.apache.hbase.thirdparty.com.google.protobuf.RpcChannel;
+import org.apache.hbase.thirdparty.com.google.protobuf.RpcController;
+import org.apache.hbase.thirdparty.com.google.protobuf.ServiceException;
 
 Review comment:
   Ditto


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


With regards,
Apache Git Services

[GitHub] [hbase] saintstack commented on a change in pull request #1280: HBASE-23799 Make our core coprocessors use shaded protobuf

2020-03-17 Thread GitBox 
saintstack commented on a change in pull request #1280: HBASE-23799 Make our 
core coprocessors use shaded protobuf
URL: https://github.com/apache/hbase/pull/1280#discussion_r393907544
 
 

 ##
 File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
 ##
 @@ -82,6 +77,12 @@
 import 
org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
 import org.apache.hbase.thirdparty.com.google.common.base.Throwables;
 
+import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
+import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos;
+import 
org.apache.hadoop.hbase.shaded.protobuf.generated.MultiRowMutationProtos.MultiRowMutationService;
+import 
org.apache.hadoop.hbase.shaded.protobuf.generated.MultiRowMutationProtos.MutateRowsRequest;
+import 
org.apache.hadoop.hbase.shaded.protobuf.generated.MultiRowMutationProtos.MutateRowsResponse;
 
 Review comment:
   Was this an oversight on our part? These should have been using the shaded 
versions all along?


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


With regards,
Apache Git Services