[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Patch Set 7:

Nice work Will!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Mike Percy (Code Review) 
Mike Percy has submitted this change and it was merged.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Add AvroKuduEventProducer to Kudu-Flume integration

This adds a KuduEventProducer that accepts events with Avro-encoded
bodies and allows the KuduSink to push them to Kudu. It only accepts
schemas of Record type that hold values of types compatible with
Kudu's types. The schema is provided to the EventProducer ahead of
time or in the event header, as a URL or as a literal.

Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Reviewed-on: http://gerrit.cloudera.org:8080/4034
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M java/kudu-flume-sink/pom.xml
A 
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java
A java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc
A 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
4 files changed, 560 insertions(+), 0 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

2016-09-05 Thread Will Berkeley (Code Review) 
Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 3:

> (1 comment)
 > 
 > didn't get to review the patch yet, just a quick comment on
 > back-compat

No problem-- that's not hard to do. I appreciate you glancing at it but you can 
dodge reviewing this for now. I mostly wanted to see if Jenkins would like the 
C++ part. Also, I realized I was slightly rude by submitting a quasi-patch for 
this when you're still assigned to the JIRA and I didn't ask :( sorry

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1113 pushed predicates not displaying on scans web page

2016-09-05 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1113 pushed predicates not displaying on scans web page
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4206/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

PS1, Line 1488: can report on predicates.
think it would be better to say something like 'report its predicates on the 
/scans page' or something

also, worth noting in this comment that the copy is necessary because 
iter->Init() ends up removing predicates that got pushed down into underlying 
iterators or somesuch


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I21821a1f7b5e4d5e5e1bdec17353ae5f3ef8e408
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Patch Set 7:

Build Started http://104.196.14.100/job/kudu-gerrit/3231/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Will Berkeley (Code Review) 
Will Berkeley has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:

Line 82: 
${project.basedir}/src/main/avro/
> this should not be in src/main it should be in src/test since this is only 
Done


Line 85:   
> nit: this line should be un-indented by 2 spaces
Done


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc
File java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc:

Line 5: {"name": "key", "type": "int"},
> I don't think we should name these fields with names that match types exact
Done


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
File 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java:

Line 136:   assertTrue("incorrect status for empty channel", status == 
Sink.Status.BACKOFF);
> assertEquals("incorrect status for empty channel", status, Sink.Status.BACK
Done


Line 138:   assertTrue("incorrect status for non-empty channel", status != 
Sink.Status.BACKOFF);
> assertEquals("incorrect status for non-empty channel", status, Sink.Status.
Done


Line 148: ArrayList columns = new ArrayList<>(3);
> nit: initial capacity should be 5
Done


Line 179:   record.setLong$(2L * i);
> See my comments about this $ syntax in the .avsc file -- let's just rename 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Will Berkeley (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..

Add AvroKuduEventProducer to Kudu-Flume integration

This adds a KuduEventProducer that accepts events with Avro-encoded
bodies and allows the KuduSink to push them to Kudu. It only accepts
schemas of Record type that hold values of types compatible with
Kudu's types. The schema is provided to the EventProducer ahead of
time or in the event header, as a URL or as a literal.

Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
---
M java/kudu-flume-sink/pom.xml
A 
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java
A java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc
A 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
4 files changed, 560 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

2016-09-05 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Patch Set 6:

(7 comments)

Found a few more test nits. I think this is nearly ready to commit now.

http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:

Line 82: 
${project.basedir}/src/main/avro/
this should not be in src/main it should be in src/test since this is only used 
in the test module.

In fact, that is the default for the generate-test-sources goal and you should 
be able to leave off the configuration section completely from here. I just 
tested this and it's true, you can remove this section.


Line 85:   
nit: this line should be un-indented by 2 spaces


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc
File java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc:

Line 5: {"name": "key", "type": "int"},
I don't think we should name these fields with names that match types exactly. 
It makes the generated Avro method names a bit weird, and people reading the 
tests might be a little confused.

Instread of "long", how about 
"longField" or something? Instead of "null", how about "nullableStringField"?

Suggestions:

* key (this is alright, or intKey)
* longField
* doubleField
* nullableStringField
* stringField


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
File 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java:

Line 136:   assertTrue("incorrect status for empty channel", status == 
Sink.Status.BACKOFF);
assertEquals("incorrect status for empty channel", status, Sink.Status.BACKOFF);


Line 138:   assertTrue("incorrect status for non-empty channel", status != 
Sink.Status.BACKOFF);
assertEquals("incorrect status for non-empty channel", status, 
Sink.Status.READY);


Line 148: ArrayList columns = new ArrayList<>(3);
nit: initial capacity should be 5


Line 179:   record.setLong$(2L * i);
See my comments about this $ syntax in the .avsc file -- let's just rename 
these fields not to conflict with type names.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes