[jira] [Comment Edited] (BEAM-2392) Avoid use of proto builder clone

["Nigel Kilmer (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/BEAM-2392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16033639#comment-16033639 ] 

Nigel Kilmer edited comment on BEAM-2392 at 6/1/17 8:38 PM:
------------------------------------------------------------

The issue is that the Java proto code generated by the Google-internal build process does not generate the clone method for proto builders. I'm currently trying to find out why this discrepancy exists, as the method is clearly documented here for external use: https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/GeneratedMessage.Builder#clone()

I wouldn't say that this is a bug, which is why I'm unsure whether you want the change. I thought it wouldn't hurt to ask though. The fix looks like this:

{code:java}
--- a/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java
+++ b/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java
@@ -168,14 +168,13 @@ class BigtableServiceImpl implements BigtableService {
     private BigtableSession session;
     private AsyncExecutor executor;
     private BulkMutation bulkMutation;
-    private final MutateRowRequest.Builder partialBuilder;
+    private final String tableName;

     public BigtableWriterImpl(BigtableSession session, BigtableTableName tableName) {
       this.session = session;
       executor = session.createAsyncExecutor();
       bulkMutation = session.createBulkMutation(tableName, executor);
-
-      partialBuilder = MutateRowRequest.newBuilder().setTableName(tableName.toString());
+      this.tableName = tableName.toString();
     }

     @Override
@@ -208,8 +207,8 @@ class BigtableServiceImpl implements BigtableService {
         KV<ByteString, Iterable<Mutation>> record)
         throws IOException {
       MutateRowRequest r =
-          partialBuilder
-              .clone()
+          MutateRowRequest.newBuilder()
+              .setTableName(tableName)
               .setRowKey(record.getKey())
               .addAllMutations(record.getValue())
               .build();
{code}


was (Author: nkilmer):
The issue is that the Java proto code generated by the Google-internal build process does not generate the clone method for proto builders. I'm currently trying to find out why this discrepancy exists, as the method is clearly documented here for external use: https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/GeneratedMessage.Builder#clone()

I wouldn't say that this is a bug, which is why I'm unsure whether you want the change. I thought it wouldn't hurt to ask though. The fix looks like this:

{code:diff}
--- a/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java
+++ b/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java
@@ -168,14 +168,13 @@ class BigtableServiceImpl implements BigtableService {
     private BigtableSession session;
     private AsyncExecutor executor;
     private BulkMutation bulkMutation;
-    private final MutateRowRequest.Builder partialBuilder;
+    private final String tableName;

     public BigtableWriterImpl(BigtableSession session, BigtableTableName tableName) {
       this.session = session;
       executor = session.createAsyncExecutor();
       bulkMutation = session.createBulkMutation(tableName, executor);
-
-      partialBuilder = MutateRowRequest.newBuilder().setTableName(tableName.toString());
+      this.tableName = tableName.toString();
     }

     @Override
@@ -208,8 +207,8 @@ class BigtableServiceImpl implements BigtableService {
         KV<ByteString, Iterable<Mutation>> record)
         throws IOException {
       MutateRowRequest r =
-          partialBuilder
-              .clone()
+          MutateRowRequest.newBuilder()
+              .setTableName(tableName)
               .setRowKey(record.getKey())
               .addAllMutations(record.getValue())
               .build();
{code}

> Avoid use of proto builder clone
> --------------------------------
>
>                 Key: BEAM-2392
>                 URL: https://issues.apache.org/jira/browse/BEAM-2392
>             Project: Beam
>          Issue Type: Improvement
>          Components: sdk-java-gcp
>    Affects Versions: 2.1.0
>            Reporter: Nigel Kilmer
>            Assignee: Nigel Kilmer
>            Priority: Minor
>
> BigtableServiceImpl uses the clone method of the MutateRowResponse proto builder here:
> https://github.com/apache/beam/blob/04e3261818aed0c129e7c715e371463bf5b5c1b8/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java#L212
> This method is not generated by the Google-internal Java proto generator, so I had to change this to get it to work with an internal project. Are you interested in adding this change to the main repository for compatibility, or would you prefer to keep the cleaner version that uses clone?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)