[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
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 BhatGerrit-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
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 BhatGerrit-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
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 SerbinGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] docs: add master permanent failure recovery workflow
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 DemboGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] c++ client: stop requiring the old gcc ABI
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tsan-suppressions: suppress various glog/gflags data races
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: upgrade LLVM to 3.9.0
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: upgrade cmake to 3.6.1
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] c++ client: adjust kudu::client::sp
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] ensure every gflag is defined outside of a namespace
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: split into dependency groups
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: patch glog to omit tests from build
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 DemboGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds
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
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 SerbinGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [website] broken link to an adoc
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 CryansGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] [website] broken link to an adoc
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
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 NolandGerrit-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
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
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
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
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
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
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 DemboTested-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
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 AlvesReviewed-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
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 ShringarpureGerrit-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
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Few ITClient improvements
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-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
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 CryansGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp
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 CryansGerrit-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
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 CryansGerrit-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
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 DemboTested-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
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 SerbinGerrit-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
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 SerbinGerrit-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
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 SerbinGerrit-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
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 AlvesGerrit-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
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 AlvesGerrit-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
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 AlvesGerrit-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
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 AlvesTested-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
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 SerbinGerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Patch gperftools to be able to work on macOS Sierra/XCode 8
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 AlvesGerrit-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
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
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 SerbinGerrit-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'
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
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 SerbinGerrit-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
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 SerbinGerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Improve and hide OperationResponse#getWriteTimestamp
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 CryansGerrit-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
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 NolandGerrit-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
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 NolandGerrit-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
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 NolandGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [twitter-demo] use AUTO FLUSH BACKGROUND session
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 SerbinGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot