date:20160921

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 1:

(1 comment)

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

PS1, Line 215:   // Set the verbosity of the commands to WARNING(2) and above.
 :   // If the user had explicitly specified verbosity, then user's
 :   // verbosity level is honored. Since '--v' depends on 
minloglevel
 :   // specifying either of them on CLI will override this setting.
 :   if 
(google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
 :   google::GetCommandLineFlagInfoOrDie("v").is_default) {
 : google::SetCommandLineOption("minloglevel",
 :  
SimpleItoa(google::GLOG_WARNING).c_str());
 :   }
> @dinesh: thank you for the feedback.  Yes, essentially this is what we can 
Also, --vmodule affects only the verbose logging stuff, like VLOG(), right 
(which will be logged only for INFO and lower level of main log level)?  It 
does not affect the main logging level like used in the LOG() directive.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 1:

(1 comment)

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

PS1, Line 215:   // Set the verbosity of the commands to WARNING(2) and above.
 :   // If the user had explicitly specified verbosity, then user's
 :   // verbosity level is honored. Since '--v' depends on 
minloglevel
 :   // specifying either of them on CLI will override this setting.
 :   if 
(google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
 :   google::GetCommandLineFlagInfoOrDie("v").is_default) {
 : google::SetCommandLineOption("minloglevel",
 :  
SimpleItoa(google::GLOG_WARNING).c_str());
 :   }
> @alexey: currently one of the gflags '--vmodule' lets us achieve what you a
@dinesh: thank you for the feedback.  Yes, essentially this is what we can use, 
but the module for that --vmodule flag is different from what I described: 
those are patterns for file names, which is cumbersome for a big codebase like 
Kudu's.  And it does not allow to dynamically control logging level for an 
already running server.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++compilation] fixed 'unused' warnings

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [c++compilation] fixed 'unused' warnings
..


Patch Set 1:

(1 comment)

Oops, it seems I pressed 'Submit' instead of reply.  Anyway, I'm definitely 
open to change it.

http://gerrit.cloudera.org:8080/#/c/4503/1/src/kudu/codegen/row_projector.cc
File src/kudu/codegen/row_projector.cc:

PS1, Line 429:   auto compat_check = [](const Schema& base1, const Schema& 
proj1,
 :  const Schema& base2, const Schema& 
proj2) {
> You find the use of a lambda to be cleaner than just surrounding the functi
Well, for some reason I didn't feel comfortable putting it under ifdef.

Besides, I didn't see any other places this method is used and there aren't any 
other analogous methods around.

Probably, there might be some performance penalties.  If you have additional 
reasons to move it under ifdef, I will definitely reconsider this.


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

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

[kudu-CR] docs: add master permanent failure recovery workflow

2016-09-21 Thread Adar Dembo (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: docs: add master permanent failure recovery workflow
..

docs: add master permanent failure recovery workflow

While testing this I filed KUDU-1620; this wasn't an issue in
master_failover-itest because it (obviously) can't do any DNS aliasing.

Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95
---
M docs/administration.adoc
1 file changed, 165 insertions(+), 14 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49d63efa76166bc548db75b0e43ae317c49f9e95
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] c++ client: stop requiring the old gcc ABI

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: c++ client: stop requiring the old gcc ABI
..

c++ client: stop requiring the old gcc ABI

With the upgrade to clang 3.9 and the transition to libc++ for TSAN, we can
now safely remove the old gcc ABI guard rail without breaking TSAN builds.

The impact on backwards compatibility is unclear. At least one Kudu vendor
has shipped a client package for Ubuntu Xenial built against the old ABI;
that package will be built against the new ABI going forward. Any
application built against the old ABI will be incompatible with said
package once the ABI changes. But, c++ client consumers are few and far
between, and the major consumer of record (Apache Impala) does not yet
build on Xenial.

Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
---
M .ycm_extra_conf.py
M CMakeLists.txt
M docs/release_notes.adoc
M python/setup.py
M src/kudu/client/client.h
M src/kudu/client/client_samples-test.sh
M src/kudu/client/samples/CMakeLists.txt
M thirdparty/build-thirdparty.sh
8 files changed, 8 insertions(+), 52 deletions(-)


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

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

[kudu-CR] tsan-suppressions: suppress various glog/gflags data races

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: tsan-suppressions: suppress various glog/gflags data races
..

tsan-suppressions: suppress various glog/gflags data races

In an upcoming patch we'll begin building more of our thirdparty
dependencies with -fsanitize=thread. Having tested that, I saw that TSAN now
flags some new data races. All of them are "benign" in that they should be
safe on x86. Nonetheless, we either need to patch glog/gflags to fix them,
or suppress them. I chose the latter for expediency.

Change-Id: I3906070cb0e202e5b841f5b163e4adc023fe3882
---
M build-support/tsan-suppressions.txt
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/util/stack_watchdog-test.cc
4 files changed, 24 insertions(+), 14 deletions(-)


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

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

[kudu-CR] thirdparty: upgrade LLVM to 3.9.0

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: upgrade LLVM to 3.9.0
..

thirdparty: upgrade LLVM to 3.9.0

In this release of LLVM we're also including the libc++ and libc++abi
projects in the tarball for the first time. They will be used in upcoming
patches for building TSAN.

Of note, this release includes native devtoolset support. See clang commit
2fcb443.

Change-Id: I7b4a19c9acb0ee76e3a27186fabb30288a28be2c
---
M src/kudu/codegen/code_generator.cc
M thirdparty/download-thirdparty.sh
D thirdparty/patches/llvm-devtoolset-toolchain.patch
M thirdparty/vars.sh
4 files changed, 8 insertions(+), 23 deletions(-)


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

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

[kudu-CR] thirdparty: upgrade cmake to 3.6.1

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: upgrade cmake to 3.6.1
..

thirdparty: upgrade cmake to 3.6.1

3.6.1 is the latest version available on the website.

The new minimum is 3.4.3 because that's the oldest version that can build
LLVM 3.9. For reference, Ubuntu 16.04 currently provides 3.5.1.

Change-Id: I8d89d02eedced4e591d0c8b6b69458abe4b62175
---
M CMakeLists.txt
M thirdparty/vars.sh
2 files changed, 4 insertions(+), 4 deletions(-)


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

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

[kudu-CR] c++ client: adjust kudu::client::sp

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: c++ client: adjust kudu::client::sp
..

c++ client: adjust kudu::client::sp

In an upcoming change we'll begin using libc++ on Linux for TSAN builds. As
macOS developers are probably aware, libc++ lacks TR1 support, so we need to
extend our existing macOS-specific workaround for TR1 shared_ptrs to apply
to any libc++ build.

This has no bearing on backwards compatibility as libc++ is still only used
in non-shipping environments.

Change-Id: I01a1ef6319c3464a8ec84f066adc885da200af70
---
M src/kudu/client/shared_ptr.h
1 file changed, 7 insertions(+), 10 deletions(-)


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

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

[kudu-CR] ensure every gflag is defined outside of a namespace

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: ensure every gflag is defined outside of a namespace
..

ensure every gflag is defined outside of a namespace

The gflags docs recommend that all gflags be defined globally, outside of any
namespace. This patch moves a couple gflags out of their respective namespaces
accordingly. It shouldn't be backwards incompatible in any way.

Change-Id: Iea2cb97539d19feae5e86f3873dab741b08e37b1
---
M src/kudu/master/master_options.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/server/server_base_options.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tserver/tablet_server_options.cc
M src/kudu/util/memory/memory.cc
M src/kudu/util/striped64-test.cc
10 files changed, 60 insertions(+), 61 deletions(-)


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

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

[kudu-CR] thirdparty: split into dependency groups

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: split into dependency groups
..

thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- instrumented_tsan: dependencies that may be instrumented, and are indeed
  in this build (with -fsanitize=thread).

These three generally map to the existing "installed", "installed-deps", and
"installed-deps-tsan" thirdparty subdirectories. There's an obvious pattern
here for future sanitizer builds (e.g. MSAN would provide an
"instrumented_msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 256 insertions(+), 195 deletions(-)


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

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

[kudu-CR] thirdparty: patch glog to omit tests from build

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: patch glog to omit tests from build
..

thirdparty: patch glog to omit tests from build

The glog tests prevent glog from being built with -fsanitize=thread[1].
Since we never run them, lets just patch them out of the build.

1. https://github.com/google/glog/issues/54

Change-Id: Iadcf062c0baa1624eabc525bc39f83d8a7135e0b
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/glog-issue-54-dont-build-tests.patch
2 files changed, 149 insertions(+), 1 deletion(-)


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

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

[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds

2016-09-21 Thread Adar Dembo (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds
..

thirdparty: use libc++ instead libstdc++ for TSAN builds

This all began because I wanted two things:
1. To use the new gcc 5 ABI on platforms that default to it (such as Ubuntu
   Xenial). Other applications compiled on these platforms will use the new
   ABI, and the fact that the Kudu client forces them to use the old ABI is
   quite unfriendly.
2. To have working local TSAN builds again, which broke following the gcc 5
   ABI transition in Xenial.

There are a number of interconnected issues at play:
A. Until 3.9, LLVM did not recognize gcc's new ABI tags, which prevented
   Kudu's codegen module from building properly against the new ABI.
B. For TSAN builds, we rebuild some thirdparty dependencies against the
   libstdc++ from thirdparty, but the LLVM libraries are not one of them.
   This may work when the system libstdc++ and the thirdparty libstdc++ are
   of the same version, but becomes increasingly unsafe as the versions
   differ. Why? Because libstdc++ only guarantees forward compatibility
   (i.e. a binary compiled against an old libstdc++ can be used with a new
   libstdc++). As an example, on Xenial the two libraries are more than a
   major version apart.
C. Continuing B, libstdc++ from gcc 5 actually breaks backward compatibility
   for certain C++11-only symbols by moving them to an inline namespace
   (e.g. std::error_category is now std::_V2::error_category). The LLVM
   libraries use these symbols, which means LLVM built against a gcc 5
   libstdc++ cannot link against the older libstdc++ in thirdparty.
D. As the libstdc++ in thirdparty is from gcc 4, it is not multilib and does
   not provide new ABI symbols (e.g. std::__cxx11::string). Meaning, if the
   rest of Kudu tried to use the new ABI, TSAN builds would fail because the
   libstdc++ in use lacks new ABI symbols.

After upgrading LLVM, the path of least resistance was to upgrade libstdc++
in thirdparty, but what a saga that turned out to be. After much trial and
error, I gave up; I could not build libstdc++ from gcc 5 with clang, and we
must use clang to realize the latest -fsanitize=thread support.

Are there any alternatives? Well, we can follow Chromium's lead and use
libc++ for TSAN instead of libstdc++. I think this makes sense for several
reasons:
- The LLVM build, such as it is, is much more friendly than gcc's build.
  Building libstdc++ out of all of gcc was always a little hacky.
- There's at least one large open-source project (Chromium) that's
  successfully gone down this path.

That brings us to this patch, which is largely about replacing libstdc++
with libc++. Here are additional interesting details:
o We now build entire set of TSAN-duplicated dependencies with
  -fsanitize=thread, not just protobuf. It doesn't affect correctness much
  either way, but it's simpler and an easier concept to extend to future
  sanitizers that DO care (e.g. MSAN).
o We now build LLVM twice: once against the system libstdc++ for build tools
  and the regular LLVM libraries, and a second time against libc++ for
  instrumented LLVM libraries. The first build is a little hokey: it'd be
  more "pure" to build LLVM three times: once for build tools, once for LLVM
  libraries, and once for instrumented LLVM libraries. But these builds are
  super long so we optimize by combining the first two. The downside is that
  the first build now places build tools in 'installed-deps' instead of
  'installed'. I played around with placing build tools in 'installed' while
  placing the libraries in 'installed-deps', but found that to be too hacky.
o The full thirdparty build is now quite a bit longer on account of the
  second LLVM library build. I tried to mitigate this by reducing the number
  of extra cruft built each time. An upcoming patch will address this
  further by splitting thirdparty into separate modules.
o libc++ depends on libc++abi, so we build that first.
o The libc++ and libc++abi builds are done standalone rather than with the
  LLVM library build, because it isn't possible to do them together AND have
  the LLVM libraries depend on libc++.
o build_python may now be invoked more than once, so I've changed it to be
  idempotent within the same run of build-thirdparty.sh.

Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run-test.sh
M build-support/run_dist_test.py
M cmake_modules/FindPmem.cmake
M src/kudu/codegen/CMakeLists.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/libstdcxx-fix-string-dtor.patch
D thirdparty/patches/libstdcxx-fix-tr1-shared-ptr.patch
M thirdparty/vars.sh
12 files

[kudu-CR] [c++compilation] fixed 'unused' warnings

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [c++compilation] fixed 'unused' warnings
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4503/1/src/kudu/codegen/row_projector.cc
File src/kudu/codegen/row_projector.cc:

PS1, Line 429:   auto compat_check = [](const Schema& base1, const Schema& 
proj1,
 :  const Schema& base2, const Schema& 
proj2) {
You find the use of a lambda to be cleaner than just surrounding the function 
definition with #ifndef NDEBUG? Okay...


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

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

[kudu-CR] [website] broken link to an adoc

2016-09-21 Thread Will Berkeley (Code Review)

Will Berkeley has posted comments on this change.

Change subject: [website] broken link to an adoc
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8fe88073807eceb8927d6ca7ddf109bd0a9fd9d3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [website] broken link to an adoc

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has uploaded a new change for review.

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

Change subject: [website] broken link to an adoc
..

[website] broken link to an adoc

Change-Id: I8fe88073807eceb8927d6ca7ddf109bd0a9fd9d3
---
M docs/release_notes.adoc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8fe88073807eceb8927d6ca7ddf109bd0a9fd9d3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

2016-09-21 Thread Brock Noland (Code Review)

Brock Noland has posted comments on this change.

Change subject: [WIP] KUDU-1563. Add support for INSERT IGNORE
..


Patch Set 2:

(12 comments)

Still have work todo, but wanted to wanted to get these comments out of my 
buffer.

http://gerrit.cloudera.org:8080/#/c/4491/2/python/kudu/__init__.py
File python/kudu/__init__.py:

PS2, Line 18: fr
> python client needs a test?
Yep as noted in my WIP commit message.


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

Line 1830: ASSERT_TRUE(insert == nullptr) << "Successful insert should take 
ownership";
> I don't this this assertion is doing anything useful. Whether or not the in
Yeah too be honest, I just copied that portion of the assertion from the test 
directly above.

Should I remove it?


Line 1842: // INSERT IGNORE does not result in error on duplicate
> nit" rephrase this comment a bit better?
Done


Line 1848: vector rows;
> anyway to consolidate the duplicated code below
Done


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:

Line 130:   case 9:
> curious is this actually being used somewhere in the test?
I think the point of this test is to just execute the Add() method?


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

Line 313: const uint8_t* 
prototype_row_storage,
> incorrect wrapping. see https://google.github.io/styleguide/cppguide.html#F
Done


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:

Line 173: // Used inserting a row and ignoring any duplicate row errors
> missing a word?
Done


Line 174: INSERT_IGNORE = 10;
> can you move this next to the plain types at the beginning, but still keep 
Done


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:

Line 66:   Status InsertIgnore(const KuduPartialRow& row) {
> is this called anywhere?
Should be when I finish the tests.


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:

Line 44:   void SetInsertIgnoreSucceeded();
> the impl of this does nothing but resetting the OperationResultPB, do we re
If we don't reset to OperationResultPB, I get a seg fault. This part I don't 
really understand so I'd be very very open to suggests or even just 
clarifications if:

1. I am doing the right thing.
2. If the members of OperationResultPB aren't set, what happens?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

Line 402:   Status InsertOrInsertIgnoreOrUpsertUnlocked(WriteTransactionState 
*tx_state,
> I'm torn whether we should change the name of this function to add the igno
I did this but didn't like it. I am going to revert.


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_metrics.h
File src/kudu/tablet/tablet_metrics.h:

PS2, Line 50: scoped_refptr rows_insert_ignored;
> do we really need this in tablet metrics? what would be the use?
Yeah I can see folding this into rows_Inserted. At the same time, I like more 
transparency. If you feel strongly I can remove.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [c++compilation] fixed 'unused' warnings

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has uploaded a new change for review.

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

Change subject: [c++compilation] fixed 'unused' warnings
..

[c++compilation] fixed 'unused' warnings

Use DCHECK() instead of DCHECK_NOTNULL() as recommended by
glog/logging.h to avoid compilation warnings in release configuration.

An example of previously emitted warning:
src/kudu/common/wire_protocol.cc:599:18:
warning:
  expression result unused [-Wunused-value]
  DCHECK_NOTNULL(dst_schema);
 ^~
thirdparty/installed-deps/include/glog/logging.h:1044:30:
note:
  expanded from macro 'DCHECK_NOTNULL'

Also, moved schema partitioning compatibility function under the
ifdef to fix the unused function warning.

Change-Id: If0a59ef51e5c5ea8be89109a48a57dc5abfde646
---
M src/kudu/client/meta_cache.h
M src/kudu/codegen/row_projector.cc
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/consensus.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/rpc/rpc.cc
6 files changed, 62 insertions(+), 64 deletions(-)


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

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

[kudu-CR] [util] minor clean-up on kudu::Subprocess

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [util] minor clean-up on kudu::Subprocess
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4502/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

PS1, Line 353: unique_ptr< DIR, std::function >
Nit: remove the spaces separating the triangular brackets from other stuff 
(i.e. unique_ptr> fd_dir).

Also, the compiler can't infer the second argument on its own the way it did 
when this used a shared_ptr?


PS1, Line 445:   if (state_ != kRunning) {
 : return Status::IllegalState("Sub-process is not running");
 :   }
I don't know about this. My impression is that the intent here was to be 
symmetric w.r.t. CheckAndOffer() and other similar functions; that is, to start 
every such function with an assertion that we're in the right state. Likely 
they could be downgraded to DCHECKs though.

Same for Start().


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

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

[kudu-CR] [util] minor clean-up on kudu::Subprocess

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has uploaded a new change for review.

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

Change subject: [util] minor clean-up on kudu::Subprocess
..

[util] minor clean-up on kudu::Subprocess

Do not call CHECK_EQ() in case where it's possible to report
on error via the return value.

Change-Id: Idd058382e4519b323aebb4c992d9088496a341cc
---
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/util/subprocess.cc
2 files changed, 20 insertions(+), 12 deletions(-)


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

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

[kudu-CR] [java client] Reinstate KUDU-1364's behavior, fix NPE

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has uploaded a new change for review.

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

Change subject: [java client] Reinstate KUDU-1364's behavior, fix NPE
..

[java client] Reinstate KUDU-1364's behavior, fix NPE

When d5082d8 tried to fix the client2tablets leak, it also undid the work
from KUDU-1364, while also adding new problems.

This patch brings back the caching of replica locations even when getting
TS disconnections by not purging the RemoteTablet caches on disconnection.
Instead, it is now done by the retried RPCs themselves after TabletClient
detects an uncaughtException, similarly to how it was calling
demoteAsLeaderForAllTablets before.

The NPE is fixed with a null check, it's an unfortunate race. I spent some
time trying to come up with a simple test but failed. ITClient has found
the issue before so we know we have _some_ coverage.

Change-Id: I8e0ed23fbf4c655037b77173a187c3fa11de4f63
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
2 files changed, 23 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e0ed23fbf4c655037b77173a187c3fa11de4f63
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [java client] Few ITClient improvements
..


[java client] Few ITClient improvements

ITClient has been flaky for a while now, mostly due to the "Row count regressed"
issue. I fixed it by using snapshot timestamps, which made me refactor how we 
build
scanners, which made me add a new counting method in BaseKuduTest.

I continued running the test and saw other issues. Some unchecked errors were 
not
killing the test, so I added an UncaughtExceptionHandler. I also saw invalid
scanner sequence ID errors that are normal due to how this test runs that were 
killing
the test. Finally, I converted some plain Exceptions into KuduExceptions which 
gave
us access to their Status.

Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Reviewed-on: http://gerrit.cloudera.org:8080/4489
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
2 files changed, 68 insertions(+), 26 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Fix an NPE in KuduException

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [java client] Fix an NPE in KuduException
..


[java client] Fix an NPE in KuduException

Saw this in a Jenkins run and also running ITClient on my machine.

Change-Id: Iceddc6931e8d3a8cb807657fc5c0804f7052e48f
Reviewed-on: http://gerrit.cloudera.org:8080/4488
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
1 file changed, 6 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iceddc6931e8d3a8cb807657fc5c0804f7052e48f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


[java client] Improve and hide OperationResponse#getWriteTimestamp

That method was returning a HT-encoded ts, not a ts in microseconds. It's
also not meant for public consumption just yet, the same way
AbstractKuduScannerBuilder#snapshotTimestampRaw is.

Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Reviewed-on: http://gerrit.cloudera.org:8080/4487
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
Reviewed-by: Adar Dembo 
---
M 
java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHybridTime.java
3 files changed, 8 insertions(+), 8 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [web-ui] KUDU-1619 Separate tables for live and dead tservers on /tablet-servers

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [web-ui] KUDU-1619 Separate tables for live and dead tservers 
on /tablet-servers
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4450/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

PS5, Line 111:   if (!live_tserver_rows.empty()) {
 :*output << "Live Tablet Servers\n";
 :*output << "\n";
 :*output << "UUIDTime since 
heartbeatRegistration\n";
 :*output << JoinStrings(live_tserver_rows, "\n");
 :*output << "\n";
 :   }
Consider making a function out of it (e.g., a lambda function) and re-using it 
for generating both tables.

Something like

  auto generate_table = [](const vector& rows,
   const string& header,
   ostream* output) {
if (!rows.empty()) {
  *output << "" << header << "\n";
  *output << "\n";
  *output << "UUIDTime since 
heartbeatRegistration\n";
  *output << JoinStrings(rows, "\n");
  *output << "\n";
}
  };

  generate_table(live_tserver_rows, "Live Tablet Servers", output);
  generate_table(dead_tserver_rows, "Dead Tablet Servers", output);


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I479fad5c2db61949f7d67bde7901e7a59c60b786
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ninad Shringarpure 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java client] Few ITClient improvements
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

PS1, Line 85: @InterfaceAudience.Private
> If you allow me I'll do this in a separate patch that also sets up the rele
Sure.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

PS1, Line 85: @InterfaceAudience.Private
> Oh, I misunderstood.
If you allow me I'll do this in a separate patch that also sets up the release 
notes for the next version.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Few ITClient improvements
..

[java client] Few ITClient improvements

ITClient has been flaky for a while now, mostly due to the "Row count regressed"
issue. I fixed it by using snapshot timestamps, which made me refactor how we 
build
scanners, which made me add a new counting method in BaseKuduTest.

I continued running the test and saw other issues. Some unchecked errors were 
not
killing the test, so I added an UncaughtExceptionHandler. I also saw invalid
scanner sequence ID errors that are normal due to how this test runs that were 
killing
the test. Finally, I converted some plain Exceptions into KuduExceptions which 
gave
us access to their Status.

Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
---
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
2 files changed, 68 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Few ITClient improvements
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java:

PS1, Line 173: KuduScanner scanner = scanBuilder.build();
 : while (scanner.hasMoreRows()) {
 :   count += scanner.nextRows().getNumRows();
 : }
 : return count;
> Am I missing something? Why doesn't the following work:
You're better at this than me.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

PS1, Line 85: @InterfaceAudience.Private
> My previous comment was an attempt at convincing you that we don't need to 
Oh, I misunderstood.

I think it should be documented anyway. It's cheap, it's a good habit to get 
into, and it leaves a paper trail for us later.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Few ITClient improvements
..

[java client] Few ITClient improvements

ITClient has been flaky for a while now, mostly due to the "Row count regressed"
issue. I fixed it by using snapshot timestamps, which made me refactor how we 
build
scanners, which made me add a new counting method in BaseKuduTest.

I continued running the test and saw other issues. Some unchecked errors were 
not
killing the test, so I added an UncaughtExceptionHandler. I also saw invalid
scanner sequence ID errors that are normal due to how this test runs that were 
killing
the test. Finally, I converted some plain Exceptions into KuduExceptions which 
gave
us access to their Status.

Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
---
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
2 files changed, 67 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Few ITClient improvements

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Few ITClient improvements
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java:

Line 155: scanner.close();
> Hmm, didn't know we need to explicitly close. Guess I'm too used to the C++
You're right, here it's not needed. I copied the other count method (the async 
one) which was unnecessarily closing the scanner.
You do need to close a scanner if you're not going to use it all the way 
through, else you leave junk in the TS. Closing it here is just a no-op on the 
client side since it knows the TS closed it when it ran out of data.


PS1, Line 173: KuduScanner scanner = scanBuilder.build();
 : while (scanner.hasMoreRows()) {
 :   count += scanner.nextRows().getNumRows();
 : }
 : return count;
> Can you use countRowsInScan() here? Maybe elsewhere too?
Not without some refactoring to make them accept predicates... Seems low gain.


http://gerrit.cloudera.org:8080/#/c/4489/1/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java:

PS1, Line 107:   
thread.setUncaughtExceptionHandler(uncaughtExceptionHandler);
> You find this cleaner than explicitly catching RuntimeException in writerTh
Yes, yes. I was burned too often by dumb programming bug that'd show up in 
those threads that wouldn't kill the test.


PS1, Line 290:   if (resp == null) {
 : return false;
 :   }
> Why would the result of session.apply() be null, or session.flush() include
If you manual flush, on apply you get null. There's a jira about this I think.


Line 295:   if (writeTimestamp != 0) {
> Under what conditions will it be 0? The very first write?
Doc was saying that it could be 0, over in the other patch David says it can't, 
so I'll remove this check.


PS1, Line 419: so when we get back with the same one
> Nit: this fragment is confusing. What does it mean to "get back with the sa
I'll clarify. Basically, if you reconnect with the same TS, and your previous 
try was successful, then you get the error message.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b5ddca26b66e9fc1f737aaacf98df340f0b9024
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

PS1, Line 85: @InterfaceAudience.Private
> So what's the verdict? To doc or not? I don't see any release notes change 
My previous comment was an attempt at convincing you that we don't need to doc 
it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

PS1, Line 85: @InterfaceAudience.Private
> Yeah, although I doubt anybody is using this. I can also change the method'
So what's the verdict? To doc or not? I don't see any release notes change in 
PS2.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Fix an NPE in KuduException

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Fix an NPE in KuduException
..

[java client] Fix an NPE in KuduException

Saw this in a Jenkins run and also running ITClient on my machine.

Change-Id: Iceddc6931e8d3a8cb807657fc5c0804f7052e48f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
1 file changed, 6 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iceddc6931e8d3a8cb807657fc5c0804f7052e48f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..

[java client] Improve and hide OperationResponse#getWriteTimestamp

That method was returning a HT-encoded ts, not a ts in microseconds. It's
also not meant for public consumption just yet, the same way
AbstractKuduScannerBuilder#snapshotTimestampRaw is.

Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
---
M 
java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestHybridTime.java
3 files changed, 8 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(2 comments)

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

Line 81:* @return a long representing a HybridClock-encoded timestamp,
> we call it hybridtime elsewhere. also iirc we actually always return the wr
I'll change the HybridClock in AbstractKuduScannerBuilder too then.
And it looks like you're right, the ts is always there.


PS1, Line 85: @InterfaceAudience.Private
> Should probably add this to the release notes for 1.1.0.
Yeah, although I doubt anybody is using this. I can also change the method's 
name in a backward compatible way.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..


Patch gperftools to be able to work on macOS Sierra/XCode 8

Apparently gperftools needs to be patched for tcmalloc to work
with the new clang version that ships with XCode 8, but the fix
hasn't landed yet.

This patch fixes that and was copied verbatim from:
https://github.com/gperftools/gperftools/issues/827

Note that this fix is virtually identical to the one merged into
jemalloc, which had the same problem:
https://github.com/jemalloc/jemalloc/pull/427/commits/19c9a3e828ed46f1576521c264640e60bd0cb01f

Finally it's noteworthy that this is an macOS/OS X only
fix and, as such, doesn't affect released binaries.

Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Reviewed-on: http://gerrit.cloudera.org:8080/4495
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M thirdparty/download-thirdparty.sh
A 
thirdparty/patches/gperftools-issue-827-add_get_default_zone_to_osx_libc_override.patch
2 files changed, 50 insertions(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [client-test] one more test for AUTO FLUSH BACKGROUND

2016-09-21 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [client-test] one more test for AUTO_FLUSH_BACKGROUND
..

[client-test] one more test for AUTO_FLUSH_BACKGROUND

An additional test for the AUTO_FLUSH_BACKGROUND flush mode:
verify that it's safe to perform synchronous and/or asynchronous
flush while having the auto-flusher thread running in the background.

Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
---
M src/kudu/client/client-test.cc
1 file changed, 22 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [client-test] one more test for AUTO FLUSH BACKGROUND

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [client-test] one more test for AUTO_FLUSH_BACKGROUND
..


Patch Set 3:

(1 comment)

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

Line 2435:   const size_t kIterNum = AllowSlowTests() ? 1024 : 256;
> I apologize for not saying the first time around.
I should have asked for the driver for your request :)

Sure.  The essence of this test is to have explicit and background flushes run 
in parallel, having some contention sometimes and making sure no errors 
appears.  There is no need to insert a lot of rows -- in that sense there is no 
difference having huge or small buffers to flush.

Yes, I'll add a check to verify that rows have landed in the DB.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [twitter-demo] use AUTO FLUSH BACKGROUND session

2016-09-21 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [twitter-demo] use AUTO_FLUSH_BACKGROUND session
..

[twitter-demo] use AUTO_FLUSH_BACKGROUND session

Changed the twitter demo application to use AUTO_FLUSH_BACKGROUND
flush mode instead of MANUAL_FLUSH mode.

Change-Id: I497c1265df132fc8ea4e635475d0d669eca21646
---
M src/kudu/twitter-demo/CMakeLists.txt
M src/kudu/twitter-demo/insert_consumer.cc
M src/kudu/twitter-demo/insert_consumer.h
3 files changed, 61 insertions(+), 115 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497c1265df132fc8ea4e635475d0d669eca21646
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..

Patch gperftools to be able to work on macOS Sierra/XCode 8

Apparently gperftools needs to be patched for tcmalloc to work
with the new clang version that ships with XCode 8, but the fix
hasn't landed yet.

This patch fixes that and was copied verbatim from:
https://github.com/gperftools/gperftools/issues/827

Note that this fix is virtually identical to the one merged into
jemalloc, which had the same problem:
https://github.com/jemalloc/jemalloc/pull/427/commits/19c9a3e828ed46f1576521c264640e60bd0cb01f

Finally it's noteworthy that this is an macOS/OS X only
fix and, as such, doesn't affect released binaries.

Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
---
M thirdparty/download-thirdparty.sh
A 
thirdparty/patches/gperftools-issue-827-add_get_default_zone_to_osx_libc_override.patch
2 files changed, 50 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4495/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS1, Line 127: 2
> Need to change this to 3.
yeah, my guess is that this wasn't a problem cuz no macOS jenkins


PS1, Line 135: gperftools-add_get_default_zone_to_osx_libc_override
> Maybe you can rename the patch so that it explicitly refers to the gperftoo
makes sense. Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tests] MANUAL FLUSH --> AUTO FLUSH BACKGROUND

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has submitted this change and it was merged.

Change subject: [tests] MANUAL_FLUSH --> AUTO_FLUSH_BACKGROUND
..


[tests] MANUAL_FLUSH --> AUTO_FLUSH_BACKGROUND

In tests, run KuduSession in AUTO_FLUSH_BACKGROUND instead of
MANUAL_FLUSH mode where appropriate.

Change-Id: Ieafc198609cceb5d6945a910364056d81786629a
Reviewed-on: http://gerrit.cloudera.org:8080/4471
Reviewed-by: David Ribeiro Alves 
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/tablet_history_gc-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/tools/ksck_remote-test.cc
11 files changed, 91 insertions(+), 101 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ieafc198609cceb5d6945a910364056d81786629a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [client-test] one more test for AUTO FLUSH BACKGROUND

2016-09-21 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [client-test] one more test for AUTO_FLUSH_BACKGROUND
..

[client-test] one more test for AUTO_FLUSH_BACKGROUND

An additional test for the AUTO_FLUSH_BACKGROUND flush mode:
verify that it's safe to perform synchronous and/or asynchronous
flush while having the auto-flusher thread running in the background.

Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
---
M src/kudu/client/client-test.cc
1 file changed, 20 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4495/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS1, Line 127: 2
Need to change this to 3.


PS1, Line 135: gperftools-add_get_default_zone_to_osx_libc_override
Maybe you can rename the patch so that it explicitly refers to the gperftools 
issue number in the name? That way it'll be easier to figure out if we still 
need it later. See "glog-issue-198-fix-unused-warnings.patch".


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Patch gperftools to be able to work on macOS Sierra/XCode 8
..

Patch gperftools to be able to work on macOS Sierra/XCode 8

Apparently gperftools needs to be patched for tcmalloc to work
with the new clang version that ships with XCode 8, but the fix
hasn't landed yet.

This patch fixes that and was copied verbatim from:
https://github.com/gperftools/gperftools/issues/827

Note that this fix is virtually identical to the one merged into
jemalloc, which had the same problem:
https://github.com/jemalloc/jemalloc/pull/427/commits/19c9a3e828ed46f1576521c264640e60bd0cb01f

Finally it's noteworthy that this is an macOS/OS X only
fix and, as such, doesn't affect released binaries.

Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/gperftools-add_get_default_zone_to_osx_libc_override.patch
2 files changed, 49 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie00d9fad36d1746aa94da3b1711353110da42d95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves

[kudu-CR] [tests] MANUAL FLUSH --> AUTO FLUSH BACKGROUND

2016-09-21 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [tests] MANUAL_FLUSH --> AUTO_FLUSH_BACKGROUND
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4471/3/src/kudu/integration-tests/all_types-itest.cc
File src/kudu/integration-tests/all_types-itest.cc:

PS3, Line 231: --consensus_max_batch_size_bytes=2097152
> yeah, in that case, I'm just afraid this will get flaky, but I guess we can
Well, it did not expose flakiness in dist_test so far.  But let's see.


http://gerrit.cloudera.org:8080/#/c/4471/5/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

Line 264: 
> if this is the next row now can you change the variable name?
Good idea!  Will do.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieafc198609cceb5d6945a910364056d81786629a
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: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

2016-09-21 Thread Dan Burkert (Code Review)

Dan Burkert has submitted this change and it was merged.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

This lists tablet/replica uuids irrespective of their states.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Reviewed-on: http://gerrit.cloudera.org:8080/4440
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [tests] MANUAL FLUSH --> AUTO FLUSH BACKGROUND

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [tests] MANUAL_FLUSH --> AUTO_FLUSH_BACKGROUND
..


Patch Set 5:

(2 comments)

oops had a unpublished comment. my apologies.

http://gerrit.cloudera.org:8080/#/c/4471/3/src/kudu/integration-tests/all_types-itest.cc
File src/kudu/integration-tests/all_types-itest.cc:

PS3, Line 231: 
> Yes, I've tested that.  As you could see from the stack trace of the debug 
yeah, in that case, I'm just afraid this will get flaky, but I guess we can 
just push and see.


http://gerrit.cloudera.org:8080/#/c/4471/5/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

Line 264:   atomic inserted_idx_;
if this is the next row now can you change the variable name?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieafc198609cceb5d6945a910364056d81786629a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [client-test] one more test for AUTO FLUSH BACKGROUND

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [client-test] one more test for AUTO_FLUSH_BACKGROUND
..


Patch Set 1:

(1 comment)

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

Line 2439:   for (size_t i = 0; i < 256; i += 2) {
this seems too short in terms ops and time, can you have this run for longer in 
slow tests?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bd5d248d4d44393689c8da81ed669395c393257
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [java client] Improve and hide 
OperationResponse#getWriteTimestamp
..


Patch Set 1:

(1 comment)

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

Line 81:* @return a long representing a HybridClock-encoded timestamp,
we call it hybridtime elsewhere. also iirc we actually always return the write 
timestamp.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8cfc6fcc1d0607a94bb8be9e5a0d53a4987920af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [WIP] KUDU-1563. Add support for INSERT IGNORE
..


Patch Set 2:

(17 comments)

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

Line 23:  * Represents a single row insert ignoring duplicate rows. Instances 
of this class should not be reused.
long line


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

PS2, Line 140: Get
long line


http://gerrit.cloudera.org:8080/#/c/4491/2/python/kudu/__init__.py
File python/kudu/__init__.py:

PS2, Line 18: fr
python client needs a test?


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

Line 1830: ASSERT_TRUE(insert == nullptr) << "Successful insert should take 
ownership";
I don't this this assertion is doing anything useful. Whether or not the 
insertion takes ownership of the contained pointer, the container is definitely 
empty since you just called release().


Line 1842: // INSERT IGNORE does not result in error on duplicate
nit" rephrase this comment a bit better?


Line 1845: ASSERT_TRUE(insertIgnore == nullptr) << "Successful insert 
ignore should take ownership";
same as above


Line 1848: vector rows;
anyway to consolidate the duplicated code below


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:

Line 130:   case 9:
curious is this actually being used somewhere in the test?


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

Line 313: const uint8_t* 
prototype_row_storage,
incorrect wrapping. see 
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:

Line 173: // Used inserting a row and ignoring any duplicate row errors
missing a word?


Line 174: INSERT_IGNORE = 10;
can you move this next to the plain types at the beginning, but still keep the 
number? you can dispense with the comment, for consistency


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:

Line 66:   Status InsertIgnore(const KuduPartialRow& row) {
is this called anywhere?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:

Line 44:   void SetInsertIgnoreSucceeded();
the impl of this does nothing but resetting the OperationResultPB, do we really 
need it?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

Line 406:   } else if (is_insert_ignore) {
> warning: don't use else after return [readability-else-after-return]
seems like it's time to have an enum to replace these bools


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

Line 402:   Status InsertOrInsertIgnoreOrUpsertUnlocked(WriteTransactionState 
*tx_state,
> warning: function 'kudu::tablet::Tablet::InsertOrInsertIgnoreOrUpsertUnlock
I'm torn whether we should change the name of this function to add the ignore 
case. It's not really a different tablet operation, like upsert was, its just a 
different way to build the response for the client. thoughts?


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:

PS2, Line 364: Note
thanks for adding this, but might be  better to expand the comment a bit.


http://gerrit.cloudera.org:8080/#/c/4491/2/src/kudu/tablet/tablet_metrics.h
File src/kudu/tablet/tablet_metrics.h:

PS2, Line 50: scoped_refptr rows_insert_ignored;
do we really need this in tablet metrics? what would be the use?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

2016-09-21 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [WIP] KUDU-1563. Add support for INSERT IGNORE
..


Patch Set 2:

At first glance I'd start by suggesting you split the patches for the various 
clients. Different people are experts in the different clients and you'll get 
parallel reviews and better and quicker feedback

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-1563. Add support for INSERT IGNORE

2016-09-21 Thread Brock Noland (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [WIP] KUDU-1563. Add support for INSERT IGNORE
..

[WIP] KUDU-1563. Add support for INSERT IGNORE

Add's `INSERT IGNORE' operation which behaves like a
normal `INSERT' except in the case when a duplicate
row error would be raised by the primary key having been
previously inserted.

Follows upsert backend/c++ patch 56c431585ed7ad07ef and
java patch 591c9ccb2a08bcfa4c.

The part I am most unsure about is returning an empty
OperationResultPB as I am not really sure where and how
the members of that operation are used. Spelunking didn't
clarify that for my newb eyes.

Also need some more tests. The python change is completely
untested.

Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
---
A java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java
M python/kudu/__init__.py
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/wire_protocol.proto
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/transactions/transaction.cc
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/write_transaction.cc
27 files changed, 322 insertions(+), 44 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [twitter-demo] use AUTO FLUSH BACKGROUND session

2016-09-21 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [twitter-demo] use AUTO_FLUSH_BACKGROUND session
..

[twitter-demo] use AUTO_FLUSH_BACKGROUND session

Changed the twitter demo application to use AUTO_FLUSH_BACKGROUND
flush mode instead of MANUAL_FLUSH mode.

Change-Id: I497c1265df132fc8ea4e635475d0d669eca21646
---
M src/kudu/twitter-demo/CMakeLists.txt
M src/kudu/twitter-demo/insert_consumer.cc
M src/kudu/twitter-demo/insert_consumer.h
3 files changed, 59 insertions(+), 115 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497c1265df132fc8ea4e635475d0d669eca21646
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

60 matches

Mail list logo