You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2017/05/10 21:45:48 UTC

[kudu-CR] [java-client] Update protoc and simplify the maven build 

Grant Henke has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6846

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 60 insertions(+), 252 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6846/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:

Line 235:         return value.toByteArray();
> this is resulting in a copy where it wasn't before. I'm a little nervous ab
I will upload a workaround to see if you prefer it.


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#2).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 64 insertions(+), 252 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java:

Line 60:     pb.setLowerBound(ByteString.copyFrom(this.lowerBound));
> So how come we have to copy these now?
oh wow...we don't. I just commented out for a quick test while mucking around. Was not supposed to be included.


http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java:

Line 264:       // TODO: we could implement a ZeroCopy approach here by subclassing LiteralByteString.
> Not sure the comment really makes as much sense now as it did before (since
Done


http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java:

Line 26: import org.apache.kudu.PbUtil;
> What's this for?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 1:

Note: Currently compiling the proto files in this build will dump warnings about the syntax field missing. I will upgrade the proto version in the rest of the codebase and add syntax=proto2 in a follow up patch.

-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 1:

(1 comment)

I suspect the test failures may be due to endianess issues in the byte buffers.

http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java:

Line 30: import com.google.protobuf.UnsafeByteOperations;
duplicate


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java:

Line 60:     pb.setLowerBound(ByteString.copyFrom(this.lowerBound));
So how come we have to copy these now?


http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java:

Line 264:       // TODO: we could implement a ZeroCopy approach here by subclassing LiteralByteString.
Not sure the comment really makes as much sense now as it did before (since before it was implied that ZeroCopyLiteralByteString subclasses LiteralByteString, and UnsafeByteOperations doesn't do that).


http://gerrit.cloudera.org:8080/#/c/6846/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java:

Line 26: import org.apache.kudu.PbUtil;
What's this for?


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6846/5/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java:

Line 266:       // We have UnsafeByteOperations, but that only supports an entire array. Here
this actually doesn't appear to be true -- UnsafeByteOperations supports unsafeWrap() with an offset and length. Still should be fine to do in a follow-up patch but at least the TODO can be made more accurate


http://gerrit.cloudera.org:8080/#/c/6846/5/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:

Line 326:       bytes = value;
don't we also need to check whether length == value.length?

maybe this isn't really worth it after all since this seems to only be used when deserializing schemas, which isn't that common an operation in the Java client (sorry for leading you astray)


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#5).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 98 insertions(+), 244 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 7: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#4).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 62 insertions(+), 253 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6846/6/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java:

Line 264
chatted offline, but posting here too. I think this TODO is still relevant -- we should try to avoid the copy. But the TODO just needs to be updated for accuracy.


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6846/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:

Line 235:         return value.toByteArray();
this is resulting in a copy where it wasn't before. I'm a little nervous about the perf here, but maybe not a big deal since scan results are using sidecars?


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/pom.xml
File java/kudu-client/pom.xml:

Line 144
> To remove this, I think you need to add attachProtoSources=false to the pro
I think this step was just trying to remove the test proto's due to some unusual behavior. See "<include>**/*test*.proto</include>".

I tested and no test proto is included after my change. Both before and after this change 25 kudu proto files are included in the final jar. I also verified with a jar in maven central.


Line 138:                     <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact>
> If you don't mind please try this out on SLES 12 SP1, Red Hat or CentOS 6.6
Will test and report back.


http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java:

Line 30: import com.google.protobuf.UnsafeByteOperations;
> duplicate
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#6).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 59 insertions(+), 257 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 7: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/pom.xml
File java/kudu-client/pom.xml:

Line 138:                     <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact>
> Will test and report back.
I ran a `mvn clean install -DskipTests` on each of these without issue.


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 4: Code-Review+2

I'd like at least one other person to take a look.

-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/pom.xml
File java/kudu-client/pom.xml:

Line 144
To remove this, I think you need to add attachProtoSources=false to the protobuf-maven-plugin configuration. Please test by extracting the kudu-client JAR and looking for .proto files inside (there should be none).


Line 138:                     <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact>
If you don't mind please try this out on SLES 12 SP1, Red Hat or CentOS 6.6, Ubuntu 14.04, and Debian 8. You don't actually need to run through the Java build (unless you want to); IIUC there will be one executable for any x86_64 Linux, so just find it and try to run it on those platforms. If it fails to run because of a missing glibc symbol version, we've got problems.


-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................


[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Reviewed-on: http://gerrit.cloudera.org:8080/6846
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 61 insertions(+), 254 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#7).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 61 insertions(+), 254 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client] Update protoc and simplify the maven build

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6846

to look at the new patch set (#3).

Change subject: [java-client] Update protoc and simplify the maven build
......................................................................

[java-client] Update protoc and simplify the maven build

This patch simplifies the Java build by using maven to
provide protoc via the Maven OS plugin and the
protocArtifact property of the Maven Protobuf plugin.

It also updates protoc to 3.3.0 and makes the required
changes due to compatibility breaks.

Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
---
M docs/installation.adoc
M java/README.md
M java/kudu-client/pom.xml
D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/pom.xml
15 files changed, 63 insertions(+), 253 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins