date:20160818

Kudu Jenkins has posted comments on this change.

Change subject: subprocess: allow Call() to read both stdout and stderr
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: better handling for positional arguments

Adar Dembo has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4013/3/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:

Line 145:const boost::optional& 
variadic_args) const {
> why an optional vector instead of just an empty vector inthe case that ther
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: basic integration test

Hello Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tool: basic integration test
..

tool: basic integration test

So far all it does is spot check some help pages, but in the future we
should augment it to test functionality too. For now that's not a big deal
because every tool function is covered in either master_migration-itest or
master_failover-itest.

Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
---
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/kudu-tool-test.cc
2 files changed, 141 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: better handling for positional arguments

Kudu Jenkins has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: better handling for positional arguments

Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: tool: better handling for positional arguments
..

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 246 insertions(+), 134 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: basic integration test

Kudu Jenkins has posted comments on this change.

Change subject: tool: basic integration test
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] subprocess: allow Call() to read both stdout and stderr

Hello Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: subprocess: allow Call() to read both stdout and stderr
..

subprocess: allow Call() to read both stdout and stderr

I'm going to use this in a new integration test for the tool.

Since the parent is now reading from two pipes, it needs to do so more
carefully. For example, if it read from stdout fully before looking at
stderr, both it and the child would deadlock if the child wrote 64k bytes
to the stderr pipe, hit the kernel limit, and got blocked.

I played around with an implementation based on poll(), but ultimately found
this one (based on libev) to be simpler.

Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
4 files changed, 163 insertions(+), 64 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If5f2be94c2e5cc0644a5bb2340adc4a71d844247
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: improve help output

Alexey Serbin has posted comments on this change.

Change subject: tool: improve help output
..


Patch Set 1: Code-Review+1

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4038/1//COMMIT_MSG
Commit Message:

Line 9: - adds blank line in between usage summary and more detail
nit for consistency: adds --> add, or justify --> justifies below instead


http://gerrit.cloudera.org:8080/#/c/4038/1/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:

Line 94:   std::stringstream msg;
nit: std::ostringstream since no input is assumed.


PS1, Line 95: []
nit: if here we are using mandatory/optional notation with <>/[] brackets, what 
[] would mean?  I would expect it to be just $0  [args] instead.


Line 96:   msg << " can be one of the following:\n";
nit: "   ..."  i.e. add a couple of spaces for better readability in 
the beginning of the line.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b4fc2da2b03edf8ce6b9998eeabd06a4fcd216d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..


Patch Set 11:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..

KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Implemented AUTO_FLUSH_BACKGROUND for the Kudu C++ client library.
KuduSession objects use RPC messenger's thread pool to schedule
background flush tasks.  In AUTO_FLUSH_BACKGROUND mode,
the KuduSession::Apply() method blocks if total amount of data
for operations not pushed to server reaches the buffer size limit.
The limit on the buffer size can be set by the
KuduSession::SetMutationBufferSpace() method.

The background flusher checks whether at least one of the following two
conditions is satisfied to determine whether it's time to flush
the accumulated write operations:
  * the over-the-waterline criterion: check whether the total size of the
freshly submitted (i.e. not-scheduled-for-flush) write operations
is over the threshold.  The threshold can be set as a percentage
of the total buffer size using the
KuduSession::SetMutationBufferFlushWatermark() method.
  * the maximum wait time criterion: check whether the previous flush
happened more than the maximum wait time ago.  The maximum wait time
can be specified in milliseconds using the
KuduSession::SetMutationBufferFlushInterva() method.

This change also addresses the following JIRA issue:
  KUDU-1376 KuduSession::SetMutationBufferSpace is not defined

Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
---
M python/kudu/tests/test_client.py
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
13 files changed, 1,124 insertions(+), 160 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/3952/11
-- 
To view, visit http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: better handling for positional arguments

Todd Lipcon has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4013/3/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:

Line 145:const boost::optional& 
variadic_args) const {
why an optional vector instead of just an empty vector inthe case that there 
are none? shouldn't the tool already know on its own whether it wanted them?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Todd Lipcon has submitted this change and it was merged.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


[doc] update on shared_ptr/scoped_refptr pros/cons

Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Reviewed-on: http://gerrit.cloudera.org:8080/4050
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M docs/contributing.adoc
1 file changed, 17 insertions(+), 8 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3952/9/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

Line 421:   flow_control_cond_.Wait();
> The condition awaits till total size of pending operations drops below the 
I meant both the periodic- and waterline-triggerd flush will notify on update 
of total_bytes_used_, if any happened.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..


Patch Set 10:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..

KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Implemented AUTO_FLUSH_BACKGROUND for the Kudu C++ client library.
KuduSession objects use RPC messenger's thread pool to schedule
background flush tasks.  In AUTO_FLUSH_BACKGROUND mode,
the KuduSession::Apply() method blocks if total amount of data
for operations not pushed to server reaches the buffer size limit.
The limit on the buffer size can be set by the
KuduSession::SetMutationBufferSpace() method or via the
--client_buffer_bytes_limit command-line flag.

The background flusher checks whether at least one of the following two
conditions is satisfied to determine whether it's time to flush
the accumulated write operations:
  * over-the-waterline criterion: check whether the total size of the
freshly submitted (i.e. not-scheduled-for-flush) write operations
is over the threshold.  The threshold can be set as a percentage
of the total buffer size using the
--client_buffer_flush_waterline_pct command-line flag.
  * maximum wait time criterion: check whether the previous flush
happened more than the maximum wait time ago.  The maximum wait time
can be specified in milliseconds using the
--client_buffer_flush_timeout_ms command-line flag.

This change also addresses the following JIRA issue:
  KUDU-1376 KuduSession::SetMutationBufferSpace is not defined

Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
---
M python/kudu/tests/test_client.py
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/util/monotime.cc
M src/kudu/util/monotime.h
13 files changed, 1,149 insertions(+), 160 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/3952/10
-- 
To view, visit http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..


Patch Set 9:

(22 comments)

http://gerrit.cloudera.org:8080/#/c/3952/9/src/kudu/client/batcher.h
File src/kudu/client/batcher.h:

Line 64:   // Takes a reference on error_collector. Creates a weak_ptr to 
'session'.
> Could you update this comment, especially WRT the error_collector?  I can't
Done


Line 115:   static int64_t GetOperationSizeInBuffer(KuduWriteOperation* 
write_op);
> This doesn't seem too useful since it just calls write_op->SizeInBuffer(). 
I didn't want to introduce an additional KuduSession::Data friend to the 
Batcher class (and it's not possible because Data is a nested class in the 
KuduSession class).

Probably, there is a more elegant solution for this.  Basically, I want to get 
wire size for KuduWriteOperation.


http://gerrit.cloudera.org:8080/#/c/3952/9/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS9, Line 555: session
> the session
Done


PS9, Line 557: maximum 
> the maximum
Done


PS9, Line 557: 10x multiplier
> 10x seems like a lot of leeway.  Could we get away with 2 or 3x?  I would c
Done


PS9, Line 2221: Status s;
> looks like everywhere this is used the call could instead be wrapped in ASS
Somehow I overlooked that :)  Thanks!


PS9, Line 2307: scenrio
> 'scenario' here and below
Done


PS9, Line 2478: previos
> previous
Done


http://gerrit.cloudera.org:8080/#/c/3952/9/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

Line 36: DEFINE_int32(client_buffer_bytes_limit, 7 * 1024 * 1024,
> Remove this and use SetMutationBufferSpace instead.
Done


Line 49: DEFINE_int32(client_buffer_flush_watermark_pct, 75,
> Instead of a gflag, this should be an option on KuduSession, like SetMutati
Done


Line 61: DEFINE_int32(client_buffer_flush_timeout_ms, 10,
> Same here, this should be an option on KuduSession.  https://kudu.apache.or
Done


PS9, Line 82: :
> wrap init list.
Done


Line 115:   messenger->ScheduleOnReactor(
> It's not clear to me why this needs to be scheduled on a reactor thread.  I
Good point!  The original design had a separate thread which was busy with all 
flush-related activity, and I just transferred it on the new scheme with 
messenger/reactor threads.  This should not be blocking, so I can safely call 
it from the same thread where CheckAndRun is being executed.


PS9, Line 154:   if (PREDICT_FALSE(!status.ok())) {
 : return;
 :   }
 :   // Check that the session is still alive to access the data 
safely.
 :   sp::shared_ptr session(weak_session.lock());
 :   if (PREDICT_FALSE(!session)) {
 : return;
 :   }
 :   // Flush mode could change during the operation.  If current 
mode
 :   // is no longer AUTO_FLUSH_BACKGROUND, then stop re-scheduling 
the task.
 :   if (PREDICT_FALSE(data_->flush_mode_ != 
AUTO_FLUSH_BACKGROUND)) {
 : return;
 :   }
 :   // Detach the batcher, schedule its flushing,
 :   // and install a new one to accumulate incoming write 
operations.
 :   data_->NextBatcher(weak_session, 0, nullptr);
> Looks like this might be able to call Run instead of duplicating?
Done


PS9, Line 205: MutexLock
> prefer to use std::lock_guard if possible.
Done


PS9, Line 392: int64_t* op_raw_size
> is this still necessary now that the size is cached?
yep -- that's the issue of visibility of a private method.  I cannot make an 
internal class a friend to the Batcher class.


Line 421:   flow_control_cond_.Wait();
> Is a flush forced somewhere in this situation, or is it just waiting for th
The condition awaits till total size of pending operations drops below the 
limit.  This is enforced by the logic of the code.  If no updates happen on 
total_bytes_used_, this will wait forever.


http://gerrit.cloudera.org:8080/#/c/3952/9/src/kudu/client/session-internal.h
File src/kudu/client/session-internal.h:

PS9, Line 111: BACKGFOUND
> BACKGROUND
Done


Line 126: ~BackgroundFlusher();
> I think this can use the default keyword instead of defining the no-op dtor
OK, I just removed it to rely on auto-generated ones.


Line 136: void CheckAndRun(const std::shared_ptr& messenger,
> Could you doc this?  I can't tell exactly what it's for, or why it chooses 
Renamed and added more comments.


PS9, Line 151: KuduSession::Data* const
> It looks like all of the methods on the background flusher that do any work
It's a reference (a pointer, actually) to the outer/enclosing object.

It's possible to keep those weak pointers as members of the BackgroundFlusher, 
but when a BackgroundFlusher object, as a sub-object of KuduSession::Data, is 
deallocated on destruction of the enclosing KuduSession::Data object, those 
members

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

Kudu Jenkins has posted comments on this change.

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


Patch Set 2:

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

-- 
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: 2
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-08-18 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 (#2).

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, as a file.

Hopefully, with the regex EventProducer, this will provide users with
a basic capability to stream into Kudu using Flume via configuration,
no coding.

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/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
A java/kudu-flume-sink/src/test/resources/testAvroEventProducer.avsc
4 files changed, 409 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/4034/2
-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1542: in some cases apply will hang.

Adar Dembo has posted comments on this change.

Change subject: KUDU-1542: in some cases apply will hang.
..


Patch Set 6:

> Lemme put it more clear:
 > 1 the "looping forever" happened when i am using "AUTO_FLUSH_BACKGROUND"
 > flush_mode.
 > 2 the timeout exception in log is like 
 > "com.stumbleupon.async.TimeoutException:
 > Timed out after 1ms when joining Defered@"
 > 3 this "looping forever" can hang quite a while ,maybe  several
 > hours . Reconnct kudu-client recover it.

That's very interesting; I wouldn't expect a background flush to hang for hours 
unless there's something broken in the client.

In KUDU-1542, JD asked if you've had a chance to retest with his newer patches. 
Did you have a chance to do that? I suspect that your fix works around a hang 
that was fixed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd33cdc5316e294e613d1b2273ef12e6b1cf687
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang 
Gerrit-HasComments: No

[kudu-CR] Add initial release notes section for 1.0.0

Adar Dembo has posted comments on this change.

Change subject: Add initial release notes section for 1.0.0
..


Patch Set 1: Code-Review+2

Provided you don't end up changing the name of the pbc dump mode.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If4b088e83c342389a53acafefad54623e9bc0aff
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tool: improve help output

Adar Dembo has posted comments on this change.

Change subject: tool: improve help output
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4038/1/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:

Line 109:   for (const auto& p : lines) {
Nit: shouldn't it be 'l' for 'line'? Or does 'p' mean something else?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b4fc2da2b03edf8ce6b9998eeabd06a4fcd216d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Convert pbc-dump over to new tool infrastructure

Adar Dembo has posted comments on this change.

Change subject: Convert pbc-dump over to new tool infrastructure
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4037/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:

Line 90
Note that removing the executable will cause the packaging builds to fail. If 
you're interested in preventing that, you should first modify install_kudu.sh 
(CDH/cdh-package.git:kudu) to stop copying kudu-pbc-dump.


http://gerrit.cloudera.org:8080/#/c/4037/1/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:

Line 253: // Returns a new "pbc" mode node.
I was thinking that modes would correspond directly to contexts. Which means, 
kudu-pbc-dump would be lumped under a "file" context along with all other 
operations that operate on a single file by name (e.g. log-dump when called on 
a segment directly).

Alternatively, we could recognize that "fs" and "file" are similar and reuse 
"fs".

What do you think?


Line 254: std::unique_ptr BuildPbcMode();
Nit: The comments for each of these aren't going to be useful. Let's just 
combine them like so:

  // Returns new nodes for each major mode.
  std::unique_ptr BuildFooMode();
  std::unique_ptr BuildBarMode();
  ...

And let's also keep the modes sorted in alphabetical order.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] tool: properly handle -version

Adar Dembo has posted comments on this change.

Change subject: tool: properly handle -version
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ac0a1d1f81bd701d8e06aab5a840eb714b52243
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tool: better handling for positional arguments

Kudu Jenkins has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: better handling for positional arguments

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: tool: better handling for positional arguments
..

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 247 insertions(+), 134 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Todd Lipcon has posted comments on this change.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Hello Kudu Jenkins,

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

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

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

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..

[doc] update on shared_ptr/scoped_refptr pros/cons

Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
---
M docs/contributing.adoc
1 file changed, 17 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Kudu Jenkins has posted comments on this change.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Alexey Serbin has posted comments on this change.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4050/1/docs/contributing.adoc
File docs/contributing.adoc:

PS1, Line 210: std::tr1::shared_ptr
> while you're at it, want to remove the tr1 reference?
sure, will post an update.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) rm CNAME

Dan Burkert has submitted this change and it was merged.

Change subject: rm CNAME
..


rm CNAME

Change-Id: Ib301f94ba8bf591d7202c95c252e2ff22c68f692
Reviewed-on: http://gerrit.cloudera.org:8080/4048
Reviewed-by: Todd Lipcon 
Tested-by: Dan Burkert 
---
D CNAME
1 file changed, 0 insertions(+), 1 deletion(-)

Approvals:
  Dan Burkert: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib301f94ba8bf591d7202c95c252e2ff22c68f692
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Todd Lipcon has posted comments on this change.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4050/1/docs/contributing.adoc
File docs/contributing.adoc:

PS1, Line 210: std::tr1::shared_ptr
while you're at it, want to remove the tr1 reference?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Todd Lipcon has posted comments on this change.

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

Patch Set 1:

(7 comments)

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

Line 74: hadoop-client
is this typical for Flume sinks? does this get shaded? Or would we expect it to
be on the classpath at runtime? Mike?

http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java
File
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java:

Line 51: * producer.upsertfalseNoWhether to
upsert or insert rows into Kudu.
- can you wrap these to multiple lines for better readability?
- an enum like 'producer.operation' with 'insert' or 'upsert' would be more
extensible (we could add insert-ignore later, for example)

PS1, Line 52: producer.schema
this sounds like it would be a schema rather than a path. perhaps schema_path?
or schema.path? not sure if there si a convention to follow for flume sinks

Line 80: FileSystem fs = FileSystem.get(path.toUri(), new
Configuration());
I think this should be path.getFileSystem(new Configuration())

Line 93: public void initialize(Event event, KuduTable table) {
unrelated suggestion: this is a very strange method name to me, since it gets
called once per event. Maybe it's too late to change it, but maybe not? If we
can't change it I think we should clarify the javadoc in the interface that
this is called once per event, not once per producer.

Line 126: String.format("Column %s is not nullable but was
decoded as null", name));
does this happen if you have a missing column? if so, I think a fallback to
_unset_ in kudu is preferable, so the kudu default takes precedence? I can't
recall if Avro has explicit nulls different from 'missing'

Line 133: row.addBoolean(name, (boolean) value);
all of these could generate ClassCastExceptions, right? Do we want to do any
up-front verification of the avro schema matching the target table schema? It's
a little strange that you get the KuduTable object each time in initialize()
instead of in configure(), which makes this tough to do. Can we improve the API
or is it too late?

--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Alexey Serbin has uploaded a new change for review.

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

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..

[doc] update on shared_ptr/scoped_refptr pros/cons

Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
---
M docs/contributing.adoc
1 file changed, 12 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin

[kudu-CR] [doc] update on shared ptr/scoped refptr pros/cons

Kudu Jenkins has posted comments on this change.

Change subject: [doc] update on shared_ptr/scoped_refptr pros/cons
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I46678a28a623c7b9c0835177e08a3f2393ed13c1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR](gh-pages) rm CNAME

Dan Burkert has posted comments on this change.

Change subject: rm CNAME
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib301f94ba8bf591d7202c95c252e2ff22c68f692
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [spark] Add insert-ignore, update, and delete as write options

Dan Burkert has submitted this change and it was merged.

Change subject: [spark] Add insert-ignore, update, and delete as write options
..


[spark] Add insert-ignore, update, and delete as write options

Change-Id: I2781104c8a655da0287977b93188e9a65e7d68bb
Reviewed-on: http://gerrit.cloudera.org:8080/4016
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley 
Reviewed-by: Todd Lipcon 
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
A java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
6 files changed, 131 insertions(+), 48 deletions(-)

Approvals:
  Will Berkeley: Looks good to me, but someone else must approve
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2781104c8a655da0287977b93188e9a65e7d68bb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] Predicate evaluation pushdown

Dan Burkert has posted comments on this change.

Change subject: Predicate evaluation pushdown
..


Patch Set 2:

(15 comments)

Overall I had a few style nits that were repeated throughout the patch:

* Put the pointer * and reference & next to the type, not next to the variable. 
 We have been consistent about this recently in Kudu, but since the column 
decoding is some of the oldest code we have some mixed styles.  Best to stick 
to next to the type going forward.

* Terminate all comments with a period, and wrap at 80 if possible or 100 max.

* User override instead of virtual + OVERRIDE

http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/cfile/binary_dict_block.cc
File src/kudu/cfile/binary_dict_block.cc:

Line 259: // TODO: implement CopyNextAndEval for all blocks
Is this still relevant?  I think you have implemented CopyNextAndEval for plain 
binary blocks.


Line 264:   // None predicates or unsatisfied predicates should return no data
terminate all comments with a period.


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/cfile/binary_dict_block.h
File src/kudu/cfile/binary_dict_block.h:

Line 140:   virtual Status SeekForward(size_t* n) OVERRIDE {
drop 'virtual' and replace OVERRIDE with override, here and above.


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/cfile/block_encodings.h
File src/kudu/cfile/block_encodings.h:

PS2, Line 150: Fetch the next n values from the block
I think this should be up to n, right?  That's at least the contract of 
CopyNextValues


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 728: // No ctx_ defined implies there is no predicate evaluation at 
the decoder level, so CopyNextAndEval will not
wrap comments at 80 columns if possible (100 max).


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/cfile/cfile_reader.h
File src/kudu/cfile/cfile_reader.h:

PS2, Line 385: prdicate
predicate


Line 387:   SelectionVector* GetPredicateSet() { return pred_set_.get();}
This seems like a strange place to put this method; why isn't it done by the 
BinaryDictBlockDecoder itself?


Line 480: 
extraneous line


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/common/column_eval_context.h
File src/kudu/common/column_eval_context.h:

Line 1: #ifndef PROJECT_COLUMN_EVAL_CONTEXT_H
Going forward, we are using '#pragma once' instead of ifdefs for new files, see 
https://en.wikipedia.org/wiki/Pragma_once


Line 17: bool _complete) :
Style, see: 
https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists


PS2, Line 17: bool &
The style guide forbids non-const reference args and fields.  This is a little 
confusing, though.  Why is it taking a boolean by reference instead of by value?


PS2, Line 24:  &
ampersand should be with the type, same with the pointer * below.  I think 
technically the style guide waffles on this, but I think we more commonly do & 
and * with the type.  
https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expressions


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 270: bool ColumnPredicate::EvaluateCell(const void *cell) const {
move this above the anonymous namespace above, the function defined in it is 
tied to Evaluate.


Line 277:   else if (predicate_type() == PredicateType::IsNotNull) {
The IsNotNull and None branches shouldn't be necessary given the prior check.


http://gerrit.cloudera.org:8080/#/c/3990/2/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:

Line 130:   bool EvaluateCell(const void *cell) const;
I think this is going to be really slow, because it forces a bunch of branches 
to happen per cell, e.g. figuring out what type the cell is and what type the 
predicate is.  I would have expected the performance on plain binary block 
decoding to get significantly slower because of this, most acutely with a 
low-selectivity predicate.

There is probably a better way to do this.  Perhaps we can figure out a way to 
have the column predicate return a fn-pointer that has already evaluated these 
branches?  Evaluate() internally does something like this, but it's obviously 
not exposed in the interface.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [spark] Add insert-ignore, update, and delete as write options

Todd Lipcon has posted comments on this change.

Change subject: [spark] Add insert-ignore, update, and delete as write options
..


Patch Set 2: Code-Review+2

Just to be clear, is this compatible? Looks like it but one of the comment 
changes seemed to indicate removal of 'kudu.upsert'. But perhaps that was just 
an out-of-date doc?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2781104c8a655da0287977b93188e9a65e7d68bb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Chris George 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Add State Message column to /tables

Todd Lipcon has posted comments on this change.

Change subject: Add State Message column to /tables
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] Add State Message column to /tables

Todd Lipcon has submitted this change and it was merged.

Change subject: Add State Message column to /tables
..


Add State Message column to /tables

The state message portion of a table's metadata is displayed in the
same table cell as the state, separated by a space. This patch puts
the state message in its own column.

Change-Id: I5d438042d760a33a815dead0f6872c005368722e
Reviewed-on: http://gerrit.cloudera.org:8080/4014
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/master/master-path-handlers.cc
1 file changed, 4 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5d438042d760a33a815dead0f6872c005368722e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) new range partitioning features blog post

Hello Mike Percy, Todd Lipcon,

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

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

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

Change subject: new range partitioning features blog post
..

new range partitioning features blog post

Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
---
A _posts/2016-08-18-new-range-partitioning-features.md
A img/2016-08-18-new-range-partitioning-features/range-and-hash-partitioning.png
A img/2016-08-18-new-range-partitioning-features/range-partitioning-on-time.png
3 files changed, 99 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR](gh-pages) new range partitioning features blog post

Dan Burkert has posted comments on this change.

Change subject: new range partitioning features blog post
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4012/4/_posts/2016-08-18-new-range-partitioning-features.md
File _posts/2016-08-18-new-range-partitioning-features.md:

PS4, Line 27: store
> typo: stored
Done


PS4, Line 51: it has been a difficult problem to
: avoid
> instead "they have been difficult to avoid"
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) rm CNAME

Hello Mike Percy, Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: rm CNAME
..

rm CNAME

Change-Id: Ib301f94ba8bf591d7202c95c252e2ff22c68f692
---
D CNAME
1 file changed, 0 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib301f94ba8bf591d7202c95c252e2ff22c68f692
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) new range partitioning features blog post

2016-08-18 Thread Will Berkeley (Code Review)

Will Berkeley has posted comments on this change.

Change subject: new range partitioning features blog post
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4012/4/_posts/2016-08-18-new-range-partitioning-features.md
File _posts/2016-08-18-new-range-partitioning-features.md:

PS4, Line 27: store
typo: stored


PS4, Line 30: instead of splitting the tablet in half
Nice. I think this will help newer users understand.


PS4, Line 51: it has been a difficult problem to
: avoid
instead "they have been difficult to avoid"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Convert pbc-dump over to new tool infrastructure

Alexey Serbin has posted comments on this change.

Change subject: Convert pbc-dump over to new tool infrastructure
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4037/1//COMMIT_MSG
Commit Message:

Line 7: Convert pbc-dump over to new tool infrastructure
nit: 'to the new tool infrastructure'?


http://gerrit.cloudera.org:8080/#/c/4037/1/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:

PS1, Line 36: oneline
This and "online" literal at line 62: do these correspond to the same 
parameter?  If yes, does it make sense to have those literals defined at one 
place?


PS1, Line 45: string
const string& instead of string?  As I see the FindOrDie() utility function 
returns a reference, so why to copy?


PS1, Line 45: "path"
Does it make sense to declare a constant for the "path" option?  I see it's 
used in a couple places at least in this file.


PS1, Line 62: "oneline"
This and 'online' parameter at line 36: do these correspond to the same 
parameter?  If yes, does it make sense to have those literals defined at one 
place?


PS1, Line 63: "path"
A constant for the name of the parameter?

Also, I see there is a flag definition for the 'oneline' parameter but there 
isn't one for 'path'.  Is it intentional?


PS1, Line 66: a
nit: it seems the 'a' article is not needed here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) new range partitioning features blog post

Dan Burkert has uploaded a new patch set (#4).

Change subject: new range partitioning features blog post
..

new range partitioning features blog post

Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
---
A _posts/2016-08-18-new-range-partitioning-features.md
A img/2016-08-18-new-range-partitioning-features/range-and-hash-partitioning.png
A img/2016-08-18-new-range-partitioning-features/range-partitioning-on-time.png
3 files changed, 99 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53504d849c2aca9ff613b11e67d1533536283931
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] Create base class for MiniCluster and ExternalMiniCluster

2016-08-18 Thread Dinesh Bhat (Code Review)

Dinesh Bhat has posted comments on this change.

Change subject: Create base class for MiniCluster and ExternalMiniCluster
..


Patch Set 5:

(1 comment)

LGTM, one nit below which you can ignore.

http://gerrit.cloudera.org:8080/#/c/3974/5/src/kudu/integration-tests/mini_cluster.h
File src/kudu/integration-tests/mini_cluster.h:

Line 91:   void ShutdownMasters();
Not exactly related to this, but while we are here, perhaps we could just flag 
a bool to Shutdown(bool masters_only) instead of another routine 
ShutDownMasters here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I62256168a9245c845e99f7253f07e69a225954bf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add initial release notes section for 1.0.0

Kudu Jenkins has posted comments on this change.

Change subject: Add initial release notes section for 1.0.0
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If4b088e83c342389a53acafefad54623e9bc0aff
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Convert pbc-dump over to new tool infrastructure

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: Convert pbc-dump over to new tool infrastructure
..

Convert pbc-dump over to new tool infrastructure

Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/tool_action.h
R src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_main.cc
4 files changed, 33 insertions(+), 32 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo

[kudu-CR] Add initial release notes section for 1.0.0

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: Add initial release notes section for 1.0.0
..

Add initial release notes section for 1.0.0

Change-Id: If4b088e83c342389a53acafefad54623e9bc0aff
---
M docs/release_notes.adoc
1 file changed, 11 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If4b088e83c342389a53acafefad54623e9bc0aff
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo

[kudu-CR] tool: improve help output

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tool: improve help output
..

tool: improve help output

- adds blank line in between usage summary and more detail
- better formatting for required parameters in usage
- better formatting for optional boolean parameters
- justify the list of subcommands

Example output with subcommands:

Usage: ./build/latest/bin/kudu  []

 can be one of the following:
  fs   Operate on a local Kudu filesystem
 pbc   Operate on a PBC (protobuf container) files
  tablet   Operate on a local Kudu replica

Example output of a leaf node:

Usage: ./build/latest/bin/kudu pbc dump  [-oneline]

Dump a PBC (protobuf container) file
path (path to PBC file) type: string default: ""
-oneline (print each protobuf on a single line) type: bool default: false

Change-Id: I0b4fc2da2b03edf8ce6b9998eeabd06a4fcd216d
---
M src/kudu/tools/tool_action.cc
1 file changed, 38 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b4fc2da2b03edf8ce6b9998eeabd06a4fcd216d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo

[kudu-CR] tool: improve help output

Kudu Jenkins has posted comments on this change.

Change subject: tool: improve help output
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b4fc2da2b03edf8ce6b9998eeabd06a4fcd216d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tool: better handling for positional arguments

Todd Lipcon has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:

PS1, Line 187:  All remaining arguments on the command line will be joined (via 
single
 :   // space delimiter) to construct the parameter value.
> I had three options:
yea, option #2 seems more natural to me (most libraries I've used present the 
leftover flags as 'argv')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: better handling for positional arguments

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: tool: better handling for positional arguments
..

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 246 insertions(+), 132 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: better handling for positional arguments

Kudu Jenkins has posted comments on this change.

Change subject: tool: better handling for positional arguments
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: properly handle -version

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tool: properly handle -version
..

tool: properly handle -version

Change-Id: I1ac0a1d1f81bd701d8e06aab5a840eb714b52243
---
M src/kudu/tools/tool_main.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
3 files changed, 12 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ac0a1d1f81bd701d8e06aab5a840eb714b52243
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo

[kudu-CR] tool: properly handle -version