beam-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nigel Kilmer (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (BEAM-2392) Avoid use of proto builder clone
Date Thu, 01 Jun 2017 20:39:04 GMT

    [ 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)

Mime
View raw message