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

2017-06-27 Thread ASF GitHub Bot (JIRA) 

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

ASF GitHub Bot commented on BEAM-2392:
--

Github user asfgit closed the pull request at:

https://github.com/apache/beam/pull/3415


> 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.4.14#64029)

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

2017-06-21 Thread ASF GitHub Bot (JIRA) 

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

ASF GitHub Bot commented on BEAM-2392:
--

GitHub user nkilmer opened a pull request:

https://github.com/apache/beam/pull/3415

[BEAM-2392] Remove uses of proto builder clone

Hi @lukecwik, could you please review this?

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nkilmer/beam remove-proto-clone

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/beam/pull/3415.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #3415


commit 46a5d0b8c9ca9ecaf7b91d3736d111f331f35a9c
Author: Nigel Kilmer 
Date:   2017-06-21T18:26:10Z

Removed uses of proto builder clone method




> 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.4.14#64029)

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

2017-06-05 Thread Luke Cwik (JIRA) 

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

Luke Cwik commented on BEAM-2392:
-

Looking at the generated source for *clone*, it seems like using it here is not 
a big win in anyway as its short hand for *newBuilder()+merge(existing 
proto/builder)*. Seems like we could take the change that you are suggesting. 
Mind opening up a PR?

I also noticed that DatastoreV1Test.java and V1TestUtil.java both use proto 
clone.

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

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

2017-06-01 Thread Nigel Kilmer (JIRA) 

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

Nigel Kilmer commented on BEAM-2392:


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

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

2017-05-30 Thread Luke Cwik (JIRA) 

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

Luke Cwik commented on BEAM-2392:
-

How did clone not work for you?

If it seems like a bug, we would gladly take a fix.

> 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: Daniel Halperin
>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)