[ANNOUNCE] Welcoming Bankim Bhavsar as Kudu committer and PMC member

2020-04-18 Thread Adar Dembo
Hi Kudu community,

I'm happy to announce that the Kudu PMC has voted to add Bankim
Bhavsar as a new committer and PMC member.

Bankim has been actively writing Kudu code for the last six months or
so. Aside from various bug fixes, his major contribution has been to
replace the existing Bloom filter predicate code with a more
full-featured implementation that should also be more robust and
efficient. One of the challenges here has been integration with Apache
Impala, and providing a common abstraction that can be used by both
codebases. This work is still ongoing but is drawing to a close pretty
soon.

Please join me in congratulating Bankim!


Re: Allow user spoofing in insecure clusters?

2017-06-21 Thread Adar Dembo
Just to be clear, the usual "sudo -u kudu ..." workaround doesn't
apply here because there's no 'kudu' UNIX user on your laptop, right?

Also, your suggestion is for _client_ applications (i.e. the 'kudu'
CLI) to look for a KUDU_USER env variable, right? If so I'm +1.


On Wed, Jun 21, 2017 at 10:52 AM, Todd Lipcon  wrote:
> Hey folks,
>
> I had the occasion yesterday of trying to run an admin tool from my laptop
> (as user 'todd') against an insecure cluster (running as user 'kudu'). The
> cluster was rejecting me because I wasn't in the admin/service ACLs, which
> was somewhat annoying.
>
> I worked around the problem by locally hacking my copy of user.cc to ignore
> my actual username and just return "kudu" :)
>
> I think it's good that we have authorization enabled even in insecure
> clusters, since it reduces test variables and prevents "accidental" usage
> of higher privileges. So, I don't think we should disable the above
> behavior.
>
> That said, it's inconvenient to have to recompile Kudu to spoof a user
> against an insecure cluster. Given that it's already easy to spoof for a
> malicious user, does anyone have an objection to making it easier for
> non-malicious users to spoof by respecting some environment variable like
> KUDU_USER or KUDU_INSECURE_USER or KUDU_SPOOF_USER or somesuch?
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: [VOTE] Apache Kudu 1.4.0 RC2

2017-06-12 Thread Adar Dembo
+1

Built TSAN and ran tests on Ubuntu 16.04.


On Mon, Jun 12, 2017 at 2:51 PM, Dan Burkert  wrote:
> +1
>
> Built and ran tests on OS X 10.10.
>
> - Dan
>
> On Mon, Jun 12, 2017 at 9:49 AM, Brock Noland  wrote:
>
>> +1 (non-binding)
>>
>> Built and ran tests
>>
>> On Mon, Jun 12, 2017 at 9:49 AM, Jean-Daniel Cryans 
>> wrote:
>> > Err sorry I meant to vote on Friday but then Happy Hour happened :)
>> >
>> > +1, ran the same verifications as for RC1.
>> >
>> > FWIW one of my test clusters hit the issue that killed RC1 after I had
>> > voted for it, so I'm glad we caught it.
>> >
>> > J-D
>> >
>> > On Mon, Jun 12, 2017 at 12:38 AM, Todd Lipcon  wrote:
>> >
>> >> Hmm, no votes so far? Let's extend the vote to tomorrow night (I guess
>> it
>> >> wasn't so nice to have most of the voting period fall on a weekend).
>> I'll
>> >> try to vote myself tomorrow as well.
>> >>
>> >> -Todd
>> >>
>> >> On Thu, Jun 8, 2017 at 7:15 PM, Todd Lipcon  wrote:
>> >>
>> >> > Hi,
>> >> >
>> >> > The Apache Kudu team is happy to announce the first release candidate
>> for
>> >> > Apache Kudu 1.4.0.
>> >> >
>> >> > Apache Kudu 1.4.0 is a minor release which offers many improvements
>> and
>> >> > fixes since the prior release. This second release candidate includes
>> the
>> >> > release notes as well as two bug fixes for issues discovered during
>> the
>> >> RC1
>> >> > voting period.
>> >> >
>> >> > The is a source-only release. The artifacts were staged here:
>> >> > https://dist.apache.org/repos/dist/dev/kudu/1.4.0-RC2
>> >> >
>> >> > Java convenience binaries in the form of a Maven repository are staged
>> >> > here:
>> >> > https://repository.apache.org/content/repositories/
>> orgapachekudu-1010/
>> >> > (note that the Java artifacts did not change since the prior RC, so
>> this
>> >> > is the same staged repository)
>> >> >
>> >> > It was built from this tag:
>> >> > https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=
>> >> > 1ed03d710ebf06afa35ee0f7813649d661f90829
>> >> >
>> >> > KEYS file:
>> >> > http://www.apache.org/dist/kudu/KEYS
>> >> >
>> >> > I'd suggest going through the README, building Kudu, and running the
>> >> > unit tests. Testing out the Maven repo would also be appreciated.
>> >> >
>> >> > The vote will run until Sunday, June 8th at 7PM PDT.
>> >> >
>> >> > Thanks,
>> >> > Todd
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Todd Lipcon
>> >> Software Engineer, Cloudera
>> >>
>>


Re: [VOTE] Apache Kudu 1.4.0 RC1

2017-06-05 Thread Adar Dembo
+1

Built on Ubuntu 16.04 in ASAN mode. All tests passed.


On Sun, Jun 4, 2017 at 10:39 PM, Todd Lipcon  wrote:
> Hi,
>
> The Apache Kudu team is happy to announce the first release candidate for
> Apache Kudu 1.4.0.
>
> Apache Kudu 1.4.0 is a minor release which offers many improvements and
> fixes since the prior release. This first release candidate does not
> contain full release notes -- I am hoping that we can start voting while
> preparing release notes, and then have an abbreviated RC2 vote if the only
> changes between RC1 and RC2 are the incorporation of docs.
>
> The is a source-only release. The artifacts were staged here:
> https://dist.apache.org/repos/dist/dev/kudu/1.4.0-RC1/
>
> Java convenience binaries in the form of a Maven repository are staged here:
> https://repository.apache.org/content/repositories/orgapachekudu-1010/
>
> It was built from this tag:
> https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=afcd412baedcd153d152bd43128ca82886630200
>
> As noted above, the release notes are work-in-progress.
>
> KEYS file:
> http://www.apache.org/dist/kudu/KEYS
>
> I'd suggest going through the README, building Kudu, and running the
> unit tests. Testing out the Maven repo would also be appreciated.
>
> The vote will run until Wednesday, June 7th at 11PM PDT. As noted above,
> we'll need to do an RC2 to incorporate release notes, so please focus your
> voting and testing on the non-doc contents here.
>
> Thanks,
> Todd


Re: Upcoming 1.4 release

2017-05-25 Thread Adar Dembo
I created branch-1.4.x at hash
5d4bf0c1586b60a23aebe47daa67d4f8bf8874f4 (This was "Fix flaky test
TestRestartWithOrphanedReplicates ").


On Mon, May 22, 2017 at 12:15 PM, Todd Lipcon  wrote:
> BTW should also add: if you've authored any major improvements or important
> bug fixes since 1.3, please start thinking about release notes (eg create
> gerrits for them or at least reply here to note them and I can take care of
> writing them en masse as RM)
>
> On Mon, May 22, 2017 at 12:12 PM, Todd Lipcon  wrote:
>
>> Hey folks,
>>
>> It's that time again -- it's been ~2 months since our last minor release
>> (1.3), so if we want to keep up the same 2-3 month cadence between
>> releases, we should branch this week and plan to release in the next few
>> weeks.
>>
>> Although we should consider it to be a "time-based" release, we do have a
>> decent amount of meaty stuff in there which will be good to get out to
>> users (276 patches at current count!)
>>
>> I'll volunteer to RM again unless anyone else is dying for the opportunity.
>>
>> Assuming everyone's on board, I'll plan to branch on Wednesday night and
>> begin doing some stress testing, looking for loose ends/blockers, etc next
>> week.
>>
>> -Todd
>>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: [VOTE] Apache Kudu 1.3.1 RC1

2017-04-13 Thread Adar Dembo
+1

I downloaded, built on Ubuntu 16.04 for TSAN, and all the C++ tests passed.

I recently filed KUDU-1975 which is (IMHO) an annoying regression, but
I don't think it should hold up the release.


On Thu, Apr 13, 2017 at 9:09 AM, Jean-Daniel Cryans  wrote:
> +1
>
> I downloaded the bits, verified the signature, md5, sha1. Built everything
> on CentOS 6.6 in release mode, ran all the tests, they passed. Same for
> Java.
>
> J-D
>
> On Wed, Apr 12, 2017 at 10:15 PM, Todd Lipcon  wrote:
>
>> Hi,
>>
>> The Apache Kudu team is happy to announce the first release candidate for
>> Apache Kudu 1.3.1.
>>
>> Apache Kudu 1.3.1 is a bug fix release which fixes critical issues
>> discovered in Apache Kudu 1.3.0. Please see the release notes for details.
>>
>> The is a source-only release. The artifacts were staged here:
>> https://dist.apache.org/repos/dist/dev/kudu/1.3.1-RC1/
>>
>> Java convenience binaries in the form of a Maven repository are staged
>> here:
>> https://repository.apache.org/content/repositories/orgapachekudu-1009/
>>
>> It was built from this tag:
>> *https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=
>> afc0f479ba6e04ec84c3c10c55be940c0784c8e1
>> > afc0f479ba6e04ec84c3c10c55be940c0784c8e1>*
>>
>> The release notes can be found here:
>> *https://github.com/apache/kudu/blob/branch-1.3.x/docs/release_notes.adoc
>> > >*
>>
>> KEYS file:
>> http://www.apache.org/dist/kudu/KEYS
>>
>> I'd suggest going through the README, building Kudu, and running the
>> unit tests. Testing out the Maven repo would also be appreciated.
>>
>> The vote will run until Monday, April 17th at 6PM PDT (more than the
>> required minimum 72 hours so folks have three weekdays and don't feel
>> pressured to vote over the weekend).
>>
>> Thanks,
>> Todd
>>


Re: Multi-word Flag Style

2017-04-10 Thread Adar Dembo
+1 to referring to gflags with dashes instead of underscores in Kudu docs.

But, I don't know whether gflags can be coerced to programmatically
emit flags with dashes (i.e. when invoked with --help) without a patch
or two. Certainly in the code we would want to retain the use of
underscores when referring to flag variables; FLAGS_foo_bar conforms
to our coding style more than something like FLAGS-foo-bar.


On Mon, Apr 10, 2017 at 11:00 AM, Alexey Serbin  wrote:
> I think it's a good move.  It would be nice to add a notice about that in
> the user-facing docs.
>
> Also, I think it would be more consistent to convert those flags altogether
> at some point to be in dash-ish form, both the code and the docs.  Maybe,
> 1.4 is a good point to do that.
>
>
> Kind regards,
>
> Alexey
>
>
>
> On 4/10/17 10:42 AM, William Berkeley wrote:
>>
>> I agree, for the reason you gave: dashes are the norm in Unix, so they
>> "feel right" for flag names.
>>
>> -Will
>>
>> On Mon, Apr 10, 2017 at 1:38 PM, Dan Burkert 
>> wrote:
>>
>>> Hi all,
>>>
>>> As of Kudu 1.3, multi-word flags can use a dash '-' separator in lieu of
>>> the underscore '_' separator.  For example,  --memory_limit_hard_bytes
>>> can
>>> now be specified as --memory-limit-hard-bytes, or even
>>> --memory_limit-hard_bytes.  Of the people I've talked to, most seem to
>>> prefer dashes to underscores in flag names, since that's been the Unix
>>> norm
>>> for a long time.
>>>
>>> Going forward, I'd like to propose that we document flag names using
>>> dashes
>>> wherever possible.  We would continue accepting underscores indefinitely,
>>> since to stop doing so would break compatibility. For the most part, this
>>> means incrementally switching the documentation to use dashes, and
>>> getting
>>> glog to output dashes in --help output.
>>>
>>> Any thoughts?
>>>
>>> - Dan
>>>
>


Re: [VOTE] Apache Kudu 1.3.0 RC2

2017-03-13 Thread Adar Dembo
+1

Built on Ubuntu 16.04.
Ran all tests in debug mode.
- tablet_peer-test failed, but the failure is tracked by the flaky
test dashboard.
Ran all tests in release mode.
- negotiation-test failed, but probably a flake too (passed the next
time through).
Examined symbols in libkudu_client_exported.so looking for any issues.

On Mon, Mar 13, 2017 at 9:50 AM, Todd Lipcon  wrote:
> Hi,
>
> The Apache Kudu team is happy to announce the second release candidate for
> Apache Kudu 1.3.0.
>
> Apache Kudu 1.3 is a minor release which adds various new features,
> improvements, bug fixes, and optimizations on top of Kudu 1.2. Highlights
> include significantly improved support for security, garbage collection of
> historical data, and lower space consumption in default configurations.
> Please see the release notes for details.
>
> Thanks to the 25 developers who contributed code or documentation to this
> release!
>
> The is a source-only release. The artifacts were staged here:
> https://dist.apache.org/repos/dist/dev/kudu/1.3.0-RC2/
>
> Java convenience binaries in the form of a Maven repository are staged here:
> https://repository.apache.org/content/repositories/orgapachekudu-1008/
> (note that the Java source did not change between RC1 and RC2 so I didn't
> rebuild)
>
> It was built from this tag:
> https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=00813f96b9cb0c9ec57a17e5c85242f7679db0e0
>
> The release notes can be found here (some links from this document will
> only work when this version is released):
> *https://github.com/apache/kudu/blob/branch-1.3.x/docs/release_notes.adoc
> *
>
> KEYS file:
> http://www.apache.org/dist/kudu/KEYS
>
> I'd suggest going through the README, building Kudu, and running the
> unit tests. Testing out the Maven repo would also be appreciated.
>
> The vote will run until Thursday, 3/16 at 10AM PDT.
>
> Thanks,
> Todd
>
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: [VOTE] Apache Kudu 1.3.0 RC1

2017-03-10 Thread Adar Dembo
I'll put out a patch to strip the NO_FATALS() that are triggering the
compilation error in gcc5.

No idea _why_ it's happening though; I think Mike will look into that.

On Fri, Mar 10, 2017 at 7:59 PM, Todd Lipcon  wrote:
> Alright, sounds fair. Are these issues being actively worked on at the
> moment by you/Dan? Would like to get a new RC out ASAP.
>
> -Todd
>
> On Fri, Mar 10, 2017 at 4:51 PM, Adar Dembo  wrote:
>
>> -1
>>
>> env_util-test.cc and env-test.cc both fail to compile on Ubuntu 16.04
>> with the default gcc (5.4.0). The errors look like this:
>>
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc: In lambda function:
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:71: error:
>> label ‘gtest_label_testnofatal_67’ used but not defined
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:108: warning:
>> label ‘gtest_label_testnofatal_73’ defined but not used
>> [-Wunused-label]
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc: In member
>> function ‘virtual void
>> kudu::env_util::EnvUtilTest_TestDiskSpaceCheck_Test::TestBody()’:
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:71: error:
>> label ‘gtest_label_testnofatal_71’ used but not defined
>> /tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:108: warning:
>> label ‘gtest_label_testnofatal_73’ defined but not used
>> [-Wunused-label]
>> src/kudu/util/CMakeFiles/env_util-test.dir/build.make:62: recipe for
>> target 'src/kudu/util/CMakeFiles/env_util-test.dir/env_util-test.cc.o'
>> failed
>>
>> I realize this is just test code, but AFAIK we have yet to ship a
>> release whose default build configuration (i.e. without NO_TESTS=1)
>> fails to build on a commonly used platform, and I'd like to avoid
>> setting that precedent. Between this and the macOS build breakages Dan
>> pointed out, I think we have enough reason to do an RC1 that builds
>> properly.
>>
>> On Fri, Mar 10, 2017 at 6:00 PM, Dan Burkert 
>> wrote:
>> > +0
>> >
>> > - env_util-test.cc fails to build on OS X 10.10 (fixed on master in
>> 97831ead
>> > <https://github.com/apache/kudu/commit/97831ead>)
>> > - external_mini_cluster-test and security-itest fail on OS X 10.10 (fixed
>> > on master in c5ec0ddb0
>> > <https://github.com/apache/kudu/commit/c5ec0ddb01da87de4a037a4879a4ef
>> 92c434930f>
>> > )
>> > - verified signature
>> >
>> > I don't think we should hold the release over macOS test issues, but if
>> we
>> > end up needing to sink it for another reason we should backport those two
>> > patches.
>> >
>> > - Dan
>> >
>> > On Thu, Mar 9, 2017 at 10:37 PM, Todd Lipcon  wrote:
>> >
>> >> On Thu, Mar 9, 2017 at 10:36 PM, Todd Lipcon  wrote:
>> >>
>> >> > Hi,
>> >> >
>> >> > The Apache Kudu team is happy to announce the first release candidate
>> for
>> >> > Apache Kudu 1.3.0.
>> >> >
>> >> > Apache Kudu 1.3 is a minor release which adds various new features,
>> >> > improvements, bug fixes, and optimizations on top of Kudu 1.2.
>> Highlights
>> >> > include significantly improved support for security, garbage
>> collection
>> >> of
>> >> > historical data, and lower space consumption in default
>> configurations.
>> >> > Please see the release notes for details.
>> >> >
>> >> > Thanks to the 25 developers who contributed code or documentation to
>> this
>> >> > release!
>> >> >
>> >> > The is a source-only release. The artifacts were staged here:
>> >> > https://dist.apache.org/repos/dist/dev/kudu/1.3.0-RC1/
>> >> >
>> >> > Java convenience binaries in the form of a Maven repository are staged
>> >> > here:
>> >> > https://repository.apache.org/content/repositories/
>> orgapachekudu-1008/
>> >> >
>> >> > It was built from this tag:
>> >> > https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=
>> >> > dfd9b491f635a89b75889f8277316c9b78143883
>> >> >
>> >> > The release notes can be found here (some links from this document
>> will
>> >> > only work when this version is released):
>> >> > *https://github.com/apache/kudu/blob/branch-1.3.x/docs/relea
>> >> se_notes.adoc
>> >> > <https://github.com/apache/kudu/blob/branch-1.3.x/docs/relea
>> >> se_notes.adoc>*
>> >> >
>> >> > KEYS file:
>> >> > http://www.apache.org/dist/kudu/KEYS
>> >> >
>> >> > I'd suggest going through the README, building Kudu, and running the
>> >> > unit tests. Testing out the Maven repo would also be appreciated.
>> >> >
>> >> > The vote will run until Monday, 3/12 at 11PM PST. This is a bit more
>> than
>> >> > the minimum 72 hours, but given the weekend, I figured it's worth
>> giving
>> >> > two full business days for people to vote.
>> >> >
>> >> Oops, just noticed the typo right after sending. This should read
>> Monday,
>> >> 3/13 at 11pm PST.
>> >>
>> >> -Todd
>> >>
>>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: [VOTE] Apache Kudu 1.3.0 RC1

2017-03-10 Thread Adar Dembo
-1

env_util-test.cc and env-test.cc both fail to compile on Ubuntu 16.04
with the default gcc (5.4.0). The errors look like this:

/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc: In lambda function:
/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:71: error:
label ‘gtest_label_testnofatal_67’ used but not defined
/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:108: warning:
label ‘gtest_label_testnofatal_73’ defined but not used
[-Wunused-label]
/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc: In member
function ‘virtual void
kudu::env_util::EnvUtilTest_TestDiskSpaceCheck_Test::TestBody()’:
/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:71: error:
label ‘gtest_label_testnofatal_71’ used but not defined
/tmp/apache-kudu-1.3.0/src/kudu/util/env_util-test.cc:65:108: warning:
label ‘gtest_label_testnofatal_73’ defined but not used
[-Wunused-label]
src/kudu/util/CMakeFiles/env_util-test.dir/build.make:62: recipe for
target 'src/kudu/util/CMakeFiles/env_util-test.dir/env_util-test.cc.o'
failed

I realize this is just test code, but AFAIK we have yet to ship a
release whose default build configuration (i.e. without NO_TESTS=1)
fails to build on a commonly used platform, and I'd like to avoid
setting that precedent. Between this and the macOS build breakages Dan
pointed out, I think we have enough reason to do an RC1 that builds
properly.

On Fri, Mar 10, 2017 at 6:00 PM, Dan Burkert  wrote:
> +0
>
> - env_util-test.cc fails to build on OS X 10.10 (fixed on master in 97831ead
> )
> - external_mini_cluster-test and security-itest fail on OS X 10.10 (fixed
> on master in c5ec0ddb0
> 
> )
> - verified signature
>
> I don't think we should hold the release over macOS test issues, but if we
> end up needing to sink it for another reason we should backport those two
> patches.
>
> - Dan
>
> On Thu, Mar 9, 2017 at 10:37 PM, Todd Lipcon  wrote:
>
>> On Thu, Mar 9, 2017 at 10:36 PM, Todd Lipcon  wrote:
>>
>> > Hi,
>> >
>> > The Apache Kudu team is happy to announce the first release candidate for
>> > Apache Kudu 1.3.0.
>> >
>> > Apache Kudu 1.3 is a minor release which adds various new features,
>> > improvements, bug fixes, and optimizations on top of Kudu 1.2. Highlights
>> > include significantly improved support for security, garbage collection
>> of
>> > historical data, and lower space consumption in default configurations.
>> > Please see the release notes for details.
>> >
>> > Thanks to the 25 developers who contributed code or documentation to this
>> > release!
>> >
>> > The is a source-only release. The artifacts were staged here:
>> > https://dist.apache.org/repos/dist/dev/kudu/1.3.0-RC1/
>> >
>> > Java convenience binaries in the form of a Maven repository are staged
>> > here:
>> > https://repository.apache.org/content/repositories/orgapachekudu-1008/
>> >
>> > It was built from this tag:
>> > https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=
>> > dfd9b491f635a89b75889f8277316c9b78143883
>> >
>> > The release notes can be found here (some links from this document will
>> > only work when this version is released):
>> > *https://github.com/apache/kudu/blob/branch-1.3.x/docs/relea
>> se_notes.adoc
>> > > se_notes.adoc>*
>> >
>> > KEYS file:
>> > http://www.apache.org/dist/kudu/KEYS
>> >
>> > I'd suggest going through the README, building Kudu, and running the
>> > unit tests. Testing out the Maven repo would also be appreciated.
>> >
>> > The vote will run until Monday, 3/12 at 11PM PST. This is a bit more than
>> > the minimum 72 hours, but given the weekend, I figured it's worth giving
>> > two full business days for people to vote.
>> >
>> Oops, just noticed the typo right after sending. This should read Monday,
>> 3/13 at 11pm PST.
>>
>> -Todd
>>


Re: TSAN false positives with std::call_once

2017-02-14 Thread Adar Dembo
Any idea why this only surfaced recently? There's a call to
std::call_once in util/logging.cc that's been there for almost a year.

In any case, I'm not a huge fan of #2. #1 and #3 both require a patch,
but I think #1 is more likely to get noticed (and reverted) upon an
LLVM 4.0 upgrade. So I think I vote for #1, though #3 is reasonable
too.

On Tue, Feb 14, 2017 at 6:09 PM, Todd Lipcon  wrote:
> BTW, I should add that libstdcxx as included in gcc 4.9.2 (devtoolset on
> el6) doesn't have this bug, since it just wraps pthread_once. So, it's
> probably not a _real_ issue since we don't use libc++ for production builds.
>
> -Todd
>
> On Tue, Feb 14, 2017 at 6:05 PM, Todd Lipcon  wrote:
>
>> I noticed that a lot of our tests have gotten flaky with a TSAN race
>> reported around SSL initialization. After digging into a bit, I found that
>> our version of libcxx has a bug in std::once where it's using a relaxed
>> load instead of an acquire load to check the flag of whether it has run yet.
>>
>> I went to go report this upstream and discovered it's actually already
>> fixed:
>> https://github.com/llvm-mirror/libcxx/commit/
>> 4dbd4fcf8626453949625bf36976e668094cfbb1
>>
>> I'd still like to resolve the race, though. A couple options:
>> 1) apply this patch to our libcxx
>> 2) use GoogleOnce instead of std::call_once
>> 3) suppress it until we upgrade to llvm 4.0 with the newer libcxx, which
>> should likely be released in a few weeks
>>
>> Any opinions?
>> -Todd
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: Use of boost non-header-only functionality

2017-01-19 Thread Adar Dembo
At one point some of us thought to (eventually) eradicate boost from
Kudu altogether, hence drawing the line on not building boost
libraries. I think that culminated with Mike's reimplementation of
boost::optional. Given that it was eventually abandoned after push
back, I guess the idea of no boost at all is a dead one.

Technically there's no issue with reintroducing boost libraries (now
that it's been vendored, as Dan said). But if we're going to do that,
perhaps we should spell out in more detail how one should answer the
question of "I need X and I see it in in at least two of std::,
boost::, or kudu::. Which do I use?".

Git commit 5b83695 has more color.

On Thu, Jan 19, 2017 at 4:33 PM, Dan Burkert  wrote:
> I think for a long time we resisted vendoring boost, and so avoiding the
> dynamically linked parts of boost was advantageous to avoid
> incompatibilities in system boost versions.  I recall we are vendoring
> boost now, so that should be less of a concern, I would think.
>
> - Dan
>
> On Thu, Jan 19, 2017 at 4:16 PM, Todd Lipcon  wrote:
>
>> Hey folks,
>>
>> I'm starting to look at implementing a 96-bit timestamp type that would be
>> compatible with Impala and Parquet's TIMESTAMP. Internally, the 96 bits are
>> a 32-bit day number (Julian day) and a 64-bit time on that day
>> (nanoseconds).
>>
>> This maps nicely to the boost::date_time::ptime data type for the purposes
>> of stringification, etc. Doing manual stringification without using a
>> library is a bit messy, since it has to account for leap years, etc, when
>> mapping the day number to a year/month/day.
>>
>> Unfortunately, we currently only use boost in a header-only fashion, and it
>> appears the necessary functionality is not header-only. I can't quite
>> recall why we have the header-only restriction - it seems we should be able
>> to static-link boost like any other library. Does anyone have a problem
>> with making that change? Was there some issue that I'm forgetting about?
>>
>> -Todd
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>


Re: [VOTE] Apache Kudu 1.2.0 RC1

2017-01-14 Thread Adar Dembo
+1

Compiled a debug build of the tests  on Ubuntu 16.04, ran them in slow
mode. All passed.

On Thu, Jan 12, 2017 at 8:24 PM, Todd Lipcon  wrote:
> Hi,
>
> The Apache Kudu team is happy to announce the first release candidate for
> Apache Kudu 1.2.0.
>
> Apache Kudu 1.2 is a minor release which adds various new features,
> improvements, bug fixes, and optimizations on top of Kudu 1.1. Please see
> the release notes for details. Thanks to the 18 developers who contributed
> code or documentation to this release!
>
> The is a source-only release. The artifacts were staged here:
> https://dist.apache.org/repos/dist/dev/kudu/1.2.0-RC1/
>
> Java convenience binaries in the form of a Maven repository are staged here:
> https://repository.apache.org/content/repositories/orgapachekudu-1006/
>
> It was built from this tag:
> https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=c3276651dcb4a4995957fe99cd75b0e174dd7e90
>
> The release notes can be found here (some links from this document will
> only work when this version is released):
> https://github.com/apache/kudu/blob/branch-1.2
> 
> .x/docs/release_notes.adoc
> 
>
> KEYS file:
> http://www.apache.org/dist/kudu/KEYS
>
> I'd suggest going through the README, building Kudu, and running the
> unit tests. Testing out the Maven repo would also be appreciated.
>
> Given the upcoming holiday weekend in the US, I think we should extend the
> voting period beyond the minimum-required 72 hours. This vote will close
> 11:59pm PST on next Tuesday, Jan 17.
>
> Thanks,
> Todd


Re: Issues with per-block compression disablement

2016-11-30 Thread Adar Dembo
Naively I'd vote for the approach involving a change to the magic
string and (of course) adding version checking into the reader code,
to prevent an additional magic string change from being necessary in
the future. I think that's the most predictable approach (i.e. someone
less familiar with the code would easily understand the motivation).

On Wed, Nov 30, 2016 at 10:31 AM, Todd Lipcon  wrote:
> Hey folks,
>
> I'm working on KUDU-1600, which has the goal of ignoring redundant
> compression options on encodings that are already inherently compressed.
> For example, when using BITSHUFFLE encoding, lz4 compression is already
> done by the bitshuffle library, and a second pass of  LZ4 or SNAPPY
> compression is just a waste of CPU.
>
> The most straightforward solution here would be to just disable it at table
> creation (or alter) time. However, the case of DICT_ENCODING is a bit more
> interesting: DICT_ENCODING operates per-block in two different modes: in
> the first mode, it dictionary-encodes the strings and uses bitshuffle to
> encode the code integers. In that case, lz4 on top is redundant. In the
> second mode, if the dictionary ends up being too-high cardinality, it falls
> back to PLAIN blocks, in which case lz4 is likely a good idea. This
> "encoding switching" is done inside the DICT_ENCODING block format: i.e the
> cfile code always goes to the dict block decoder, which internally falls
> back to using the PLAIN decoder if necessary.
>
> Note that, within a single cfile, it's possible that some blocks have done
> "fallback" whereas others are dict-encoded.
>
> Given the above, it's not a good idea to have a blanket rule "LZ4 +
> DICT_ENCODING is disallowed". Rather, we'd like to allow LZ4 + DICT, but
> only compress the DICT blocks when the fallback code is activated. This
> implies that we need two new bits of code:
>
> 1) after encoding a block, we need to call something like
> block_encoder_->ShouldCompress(compression_codec). The encoder can then
> decide whether said compression is redundant on a per-block basis.
> 2) when writing each block, then, we need to store whether or not it was
> compressed. This is where things get tricky, since the format doesn't
> currently provide for such a per-block "is_compressed' flag.
>
> So, this brings us to the subject of this email: we need to somehow evolve
> the cfile format to add such capabiltiies.
>
> Currently, if compression is enabled in a cfile, then every block has an
> 8-byte prefix  . We could
> use a special value like '0x 0x'' to indicate a "skipped
> compression" block, but it feels a bit like a hack, and wastes 8 bytes.
> Another option would be to use a shorter code like a single 0x to
> save 4 bytes.
>
> Obviously these options would break backwards-compatibility of the cfile
> format: the old reader would see these values and be confused, generating
> some kind of error about a corrupt cfile. That's not ideal, but not sure I
> see a better way around it.
>
> Another thing we could do to generate a slightly less weird error would be
> to bump the CFile version number to indicate an incompatible change, and
> then use a simple scheme like a single byte "is_compressed" at the start of
> each block. However, it turns out that our reader code doesn't currently
> check the major/minor versions in the CFile header (woops!) so we'd
> actually need to bump the 'magic' string at the start of the file.
>
> This, too, would be backwards-incompatible and prevent downgrade from a new
> version back to an old one.
>
> Any thoughts on the best way to address this issue?
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: Failing tests with leftover tmp files from java minikdc

2016-11-30 Thread Adar Dembo
We've managed to make it this far (since commit 44fb5a9 in late 2014)
while being strict; I don't think we should relax it now.

After some digging, here's what's going on. TestMiniKuduCluster failed
once and was rerun. The second run passed, but the first failed in
such a way that it left the kdc's temp directory behind. From the
console output:

01:55:50 
org.apache.kudu.client.TestMiniKuduCluster.testKerberos(org.apache.kudu.client.TestMiniKuduCluster)
01:55:50   Run 1: TestMiniKuduCluster.testKerberos:74 ? IO process
'kinit' failed: 1
01:55:50   Run 2: PASS

And the test output:

Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 11.137
sec <<< FAILURE! - in org.apache.kudu.client.TestMiniKuduCluster
testKerberos(org.apache.kudu.client.TestMiniKuduCluster)  Time
elapsed: 1.023 sec  <<< ERROR!
java.io.IOException: process 'kinit' failed: 1
at org.apache.kudu.client.MiniKdc.checkReturnCode(MiniKdc.java:362)
at org.apache.kudu.client.MiniKdc.kinit(MiniKdc.java:193)
at 
org.apache.kudu.client.MiniKuduCluster.(MiniKuduCluster.java:103)
at 
org.apache.kudu.client.MiniKuduCluster.(MiniKuduCluster.java:56)
at 
org.apache.kudu.client.MiniKuduCluster$MiniKuduClusterBuilder.build(MiniKuduCluster.java:598)
at 
org.apache.kudu.client.TestMiniKuduCluster.testKerberos(TestMiniKuduCluster.java:74)

Given that it was the constructor that threw an exception, I'm
guessing the try-with-resources in TestMiniKuduCluster doesn't bother
to close() the cluster? Dan, what do you think?

Separately, it's worth investigating why kinit failed. Maybe this was
the failure?

01:53:49.523 [DEBUG - Thread-8] (MiniKdc.java:353) kinit: Client
'testu...@krbtest.com' not found in Kerberos database while getting
initial credentials

Perhaps kadmin.local finishes add_principal before it is actually
committed to its database?

On Wed, Nov 30, 2016 at 9:06 AM, Todd Lipcon  wrote:
> I just saw a release build precommit fail with the following:
>
> *02:03:34* All tests passed, yet some left behind their test
> output:*02:03:34* krb5kdc-1480470828522
>
>
> http://104.196.14.100/job/kudu-gerrit/4926/BUILD_TYPE=RELEASE/consoleFull
>
> It looks like this is left over from the MiniKDC in Java.
>
> Are we being too strict about checking for leftover tmp stuff? Or is there
> an easy fix to prevent this?
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: kudu configuration issue

2016-10-27 Thread Adar Dembo
(dropping issues@kudu; that list is just for e-mails generated by our
JIRA bug tracker)

To change the data directories, you'll need to change the value of
--fs_data_dirs in /etc/kudu/{master,tserver}.gflagfile. Adding it to
/etc/default/kudu-master the way you did will have no effect. Make
sure the new directories exist, and that the Kudu user can
read/write/execute to them.

Background: /etc/default/kudu-{master,tserver} are shell scripts that
are sourced by the Kudu init.d scripts. The bulk of Kudu configuration
must be defined the gflagfiles, found in /etc/kudu. The reason for
this separation is because the init.d scripts also need to know about
a few parameter values, so those parameters are passed in via shell
script instead of gflagfiles.

On Thu, Oct 27, 2016 at 8:28 AM, Dinesh Bhat  wrote:
> Hi Chun-fu,
>
> You should see a filesystem laid out with subdirectories like ’tablet-meta’, 
> ‘data’, ‘consensus-meta’, etc when you ls -l on the specified path.
> Typically, the top-level-dir is specified as arg: 
> "—fs_data_dirs=/home/myname/kudu" and internally kudu figures out the 
> filesystem layout.
>
> Hope that helps.
> Tx,
>> On Oct 27, 2016, at 2:44 AM, Yang, Chun-Fu (Eric,HPSW-BTO-R&D-SH) 
>>  wrote:
>>
>> Hi,
>> I  just installed kudu on Ubuntu 14.04.1 x86_64 system.
>> After installation, kudu-master and kudu-tserver services started and can 
>> access the web ui.
>>
>> Afther that,I stop the service and  configured   
>> --fs_data_dirs=/home/myname/kudu/data in /etc/default/kudu-master
>> Then when I start kudu-master, the following message was printed in the 
>> console
>>
>> /etc/default/kudu-master: line 1: --fs_data_dirs=/home/myname/kudu/data: No 
>> such file or directory
>> But the folder  /home/myname/kudu/data do exist, I just created it before 
>> change the configuration.
>>
>> What's the problem here?
>>
>> Thanks,
>> Chunfu
>>
>


Re: Scope document for addressing block manager-related issues

2016-10-11 Thread Adar Dembo
Thanks for the feedback, everyone.

I think I've incorporated all of the feedback and I added a ton more
background information. If you've wanted to know how this stuff works
but were afraid to ask, now's your chance!

I've also added a proposed solution and roadmap at the end.


On Thu, Oct 6, 2016 at 8:05 AM, Jean-Daniel Cryans  wrote:
> On Wed, Oct 5, 2016 at 10:03 PM, Todd Lipcon  wrote:
>
>> Did some more research on Ceph as well as what RocksDB is doing, and have
>> another brainstorm idea:
>>
>> What if we added a file descriptor LRU cache to the File Block Manager?
>> Frequently accessed data would (a) probably in the block cache, or (b) at
>> least be already open in the FD LRU cache. In the case that it is neither,
>> maybe the cost of the open() syscall isn't so bad?
>>
>> One of the places where this might fail is the fact that we have to read a
>> lot of file headers/footers when a tablet is opened. Typically these reads
>> fall in the first (or last) few KB of the file. Given that, what about some
>> kind of hybrid scheme:
>>
>> - We have a "fixed-size" allocator supporting only 8KB allocations. Thus
>> there is no fragmentation issue and reclaiming space is trivial.
>> - For each block, we allocate 8KB which stores the first 4KB of data as
>> well as the last 4KB of data.
>> - The "middle" of the file is stored as a normal file system file.
>>
>> At startup, we can easily do a big fadvise() to page in all the block
>> footers and headers so that the initial load time is low (probably lower
>> than today!). Then we rely on setting ulimit relatively high (eg 100k) and
>> a pretty big LRU cache of fds so that warm blocks are kept open.
>>
>> One downside is that we still put pressure on the inode table of the file
>> system, and operations like "rm -Rf /data" are super slow. That's not a
>> huge deal, though, IMO.
>>
>
> You know my opinion on that :)
>
> Another downside is that switching block manager will have an impact on
> migration?
>
>
>>
>> -Todd
>>
>> On Wed, Oct 5, 2016 at 5:08 PM, Todd Lipcon  wrote:
>>
>> > Thanks for posting this.
>> >
>> > It's worth taking a look at what some other systems have done as well. I
>> > just spent some time looking at Ceph, and sounds like they ran into
>> similar
>> > issues and moved to a raw-disk based idea: http://www.slideshare.
>> > net/sageweil1/bluestore-a-new-faster-storage-backend-for-ceph-63311181
>> >
>> > I'll keep investigating how their actual allocator works. I dont think a
>> > raw disk is usable for Kudu, but maybe some ideas would translate.
>> >
>> > -Todd
>> >
>> > On Wed, Oct 5, 2016 at 1:53 PM, Adar Dembo  wrote:
>> >
>> >> I've written up a doc that summarizes two major issues related to hole
>> >> punching in Kudu's log block manager, as well as some approaches for
>> >> fixing them. Please take a look if you're interested in the subject;
>> >> your feedback is welcome.
>> >>
>> >> https://s.apache.org/uOOt
>> >>
>> >
>> >
>> >
>> > --
>> > Todd Lipcon
>> > Software Engineer, Cloudera
>> >
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>


Scope document for addressing block manager-related issues

2016-10-05 Thread Adar Dembo
I've written up a doc that summarizes two major issues related to hole
punching in Kudu's log block manager, as well as some approaches for
fixing them. Please take a look if you're interested in the subject;
your feedback is welcome.

https://s.apache.org/uOOt


Re: Heads-up: thirdparty tree changes may cause build breakages for a bit

2016-09-29 Thread Adar Dembo
All of the patches have been merged. Once you rebase, you'll probably
need the big hammer ('git clean -xdf thirdparty') to clean up your
local repo. Also, _PLEASE_ rebase all of your outstanding work before
pushing to gerrit; mixing code reviews from before this patch series
and after will cause long gerrit build times as well as inscrutable
build failures.

Here's a quick summary of what changed:
1. LLVM and cmake have both been upgraded (LLVM from 3.8 to 3.9 and
cmake from 3.2.3 to 3.6.1).
2. The thirdparty tree is now organized into 'src', 'build', and
'installed' subdirectories.
3. All thirdparty dependencies are now rebuilt for TSAN (previously we
were only rebuilding a subset). However, TSAN dependencies are only
built if you're doing a TSAN build, so the net thirdparty build time
should decrease for most people.
4. We now build TSAN using LLVM's libc++ instead of gcc 4.9.3
libstdc++. This means TSAN builds will now work on Ubuntu 15.10 and
16.04 (they were previously broken).
5. On supported distros, Kudu and the thirdparty tree are now built
with the "new" gcc5 ABI
(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html).


On Wed, Sep 28, 2016 at 3:10 PM, Jean-Daniel Cryans  wrote:
> Thanks Adar. On RHEL6 removing thirdparty/installed* is what did it for me.
>
> J-D
>
> On Wed, Sep 28, 2016 at 3:04 PM, Adar Dembo  wrote:
>
>> I'm working on a pretty large series of changes to Kudu's thirdparty
>> dependency tree. The first major piece
>> (https://gerrit.cloudera.org/#/c/4507) was merged last night and has
>> led to a Kudu build breakage for some people. The failure was in the
>> link step of kudu-tserver, with errors like the following:
>>
>> ../../../../../thirdparty/installed/lib/libLLVMAsmPrinter.a(
>> CodeViewDebug.cpp.o):
>> In function `llvm::CodeViewDebug::CodeViewDebug(llvm::AsmPrinter*)':
>> CodeViewDebug.cpp:(.text._ZN4llvm13CodeViewDebugC2EPNS_
>> 10AsmPrinterE+0x37):
>> undefined reference to
>> `llvm::codeview::TypeTableBuilder::TypeTableBuilder()'
>> CodeViewDebug.cpp:(.text._ZN4llvm13CodeViewDebugC2EPNS_
>> 10AsmPrinterE+0x3e):
>> undefined reference to `vtable for
>> llvm::codeview::MemoryTypeTableBuilder'
>>
>> If you see this, you can work around it by deleting
>> thirdparty/installed* and running thirdparty/build-if-necessary.sh
>> again. That's the small hammer. If that doesn't work, try the big
>> hammer: "git clean -xdf thirdparrty" followed by
>> thirdparty/build-if-necessary.sh.
>>
>> I'll be merging some additional changes in the days to come, though
>> I'll be taking care to avoid causing additional breakages. When
>> everything is committed I'll send another heads-up email.
>>


Heads-up: thirdparty tree changes may cause build breakages for a bit

2016-09-28 Thread Adar Dembo
I'm working on a pretty large series of changes to Kudu's thirdparty
dependency tree. The first major piece
(https://gerrit.cloudera.org/#/c/4507) was merged last night and has
led to a Kudu build breakage for some people. The failure was in the
link step of kudu-tserver, with errors like the following:

../../../../../thirdparty/installed/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o):
In function `llvm::CodeViewDebug::CodeViewDebug(llvm::AsmPrinter*)':
CodeViewDebug.cpp:(.text._ZN4llvm13CodeViewDebugC2EPNS_10AsmPrinterE+0x37):
undefined reference to
`llvm::codeview::TypeTableBuilder::TypeTableBuilder()'
CodeViewDebug.cpp:(.text._ZN4llvm13CodeViewDebugC2EPNS_10AsmPrinterE+0x3e):
undefined reference to `vtable for
llvm::codeview::MemoryTypeTableBuilder'

If you see this, you can work around it by deleting
thirdparty/installed* and running thirdparty/build-if-necessary.sh
again. That's the small hammer. If that doesn't work, try the big
hammer: "git clean -xdf thirdparrty" followed by
thirdparty/build-if-necessary.sh.

I'll be merging some additional changes in the days to come, though
I'll be taking care to avoid causing additional breakages. When
everything is committed I'll send another heads-up email.


Re: Kudu 1.0.1 patch release

2016-09-26 Thread Adar Dembo
+1 to a patch release.

My only opinion on the nominated patches is that I'm not sure if we
need to include the KUDU-1090 fix. IIRC it was very rare and generally
only contributed to test flakiness. But, I'm fine with including it if
others feel strongly.

On Mon, Sep 26, 2016 at 10:55 AM, Todd Lipcon  wrote:
> +1 for a bug fix release, especially to address the known crash bugs. I'll
> add one more to the running:
>
> https://gerrit.cloudera.org/#/c/4535/
>
> This fixes a crash when running on single-core systems. Again not too
> common (even most VMs have two cores these days) but it can be a blocker if
> you need to run on a single-core.
>
> -Todd
>
> On Mon, Sep 26, 2016 at 10:49 AM, Dan Burkert  wrote:
>
>> Hi all,
>>
>> Since the 1.0.0 release there have been a few issues found which may
>> warrant a bug fix on the 1.0.x line.  I'd like to get the ball rolling and
>> figure out what we might want to include in a bug release, if we decide to
>> go ahead with a bug release fix.
>>
>> Potential inclusions:
>>
>> KUDU-1652 : Partition
>> pruning / scan optimization fails with IS NOT NULL predicate on PK column
>> KUDU-1651 : tserver crash
>> when pushing predicate on dict encoded block with all null values
>> 8fc75a5c65
>> > 1df4707d0e>:
>> [java
>> client] Fix an NPE in KuduException
>> 9911c489
>> > 7b4953421b>
>>  KUDU-1623 : Properly
>> handle UPSERTS that only include PK column
>> b0b273e8
>> > d1c792e413>:
>> [java client] make DateFormat safe to use
>> 1eb2418
>> > cf905f6129>:
>> consensus: properly truncate all state when aborting operations
>> KUDU-1090  4b9d2f6
>> > 3a0941a569>:
>> relax
>> MemTracker uniqueness constraint
>>
>> It would also be nice to fix the Java client's client2tablet
>> synchronization / memory leak issue, but I'm not sure of the proper set of
>> patches to backport.  JD/David, do you have any insight on that?
>>
>> Please reply with any other commits that you would like to include.
>>
>> Most of these issues are extremely rare, or easily worked around (or both),
>> but I think in aggregate they represent enough sharp edges that waiting for
>> a 1.1 release may be painful.
>>
>> Since a few of these issues don't have a patch committed or even in review
>> yet, an RC probably can't be cut before the end of next week (October
>> 7th).  I volunteer to RM this one.  What does everyone think?
>>
>> - Dan
>>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: RFC: removing raft_consensus-test (mock-based)

2016-09-20 Thread Adar Dembo
Does this mean we should also remove gmock from thirdparty and go back to
plain gtest? Or are we still making use of other gmock features (not
related to mocking, if any)?

On Tuesday, September 20, 2016, Todd Lipcon  wrote:

> Great, glad you agree. It also feels good to delete 700 lines of code :)
>
> I included the removal in this patch: https://gerrit.cloudera.org/#/
> c/4476/
>
> Planning on moving it from "WIP" state to commitable hopefully today, but
> already worth a cursory look to make sure the direction of the refactor is
> agreeable.
>
> -Todd
>
> On Tue, Sep 20, 2016 at 10:59 AM, David Alves  > wrote:
>
> > +1, we've since broadened our coverage in other tests that those cases
> > are covered elsewhere. Plus I agree that with the queue taking more
> > responsibility mocking it becomes a hardship.
> >
> > -david
> >
> >
> > > I'm doing a bit of Consensus refactoring, and finding that the
> > raft_consensus-test mock-based test is a bit of a roadblock to
> refactoring.
> > We don't use gmock anywhere else, and so whenever I look at this code I
> > quickly develop a head-ache trying to understand what it's doing. Looking
> > through the test cases themselves, I believe all of the code paths they
> try
> > to cover are equally well covered by other test cases in
> > raft_consensus-itest. Rather than spend several hours trying to figure
> out
> > why the mock isn't behaving like the real code, I'd rather spend that
> time
> > on adding more test cases for the real code. Thoughts?
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>


Re: 'tidy-bot' now running (experimentally)

2016-09-19 Thread Adar Dembo
Personally I haven't seen much of a need for abbreviated parameter
names. I'd vote for keeping the warning.

On Mon, Sep 19, 2016 at 2:14 PM, Todd Lipcon  wrote:
> How do people feel about this warning?
>
> warning: function 'kudu::InsertLoadgen::InserterThread' has a definition
> with different parameter names
> [readability-inconsistent-declaration-parameter-name]
> void InserterThread(Generator::Mode gen_mode, int64_t seed,
> ^
> gen_seed
> src/kudu/tools/insert-generated-rows.cc:307:21: note: the definition seen
> here
> void InsertLoadgen::InserterThread(Generator::Mode gen_mode, int64_t
> gen_seed,
> ^
> src/kudu/tools/insert-generated-rows.cc:234:8: note: differing parameters
> are named here: ('seed'), in definition: ('gen_seed')
> void InserterThread(Generator::Mode gen_mode, int64_t seed,
>
>
> Is it a reasonable one to keep, or should I disable it? On the one hand, it
> might catch a bug where the order of parameters is swapped between the
> definition and declaration, or where you forgot to update the name when
> changing the definition. On the other hand, sometimes it can make sense to
> have an "abbreviated" parameter name in the function definition.
>
> -Todd
>
> On Fri, Sep 16, 2016 at 10:42 AM, Adar Dembo  wrote:
>
>> OK, let's keep it then. I changed the comment in question to be
>> "TODO(KUDU-1537)...".
>>
>> On Thu, Sep 15, 2016 at 8:18 PM, Jake Farrell  wrote:
>> > we use "TODO(bug id or committer id): msg" as the format in other
>> projects
>> > and that seems to be enough breadcrumb in most cases
>> >
>> > -Jake
>> >
>> > On Thu, Sep 15, 2016 at 11:06 PM, Todd Lipcon  wrote:
>> >
>> >> On Thu, Sep 15, 2016 at 6:50 PM, Adar Dembo  wrote:
>> >>
>> >> > In https://gerrit.cloudera.org/#/c/4435/, Tidy Bot said:
>> >> >
>> >> > Line 209:   // TODO: Should be fixed with Exactly Once semantics,
>> >> > see KUDU-1537.
>> >> > warning: missing username/bug in TODO [google-readability-todo]
>> >> >   // TODO: Should be fixed with Exactly Once semantics, see
>> >> KUDU-1537.
>> >> >   ^
>> >> >   // TODO(unknown): Should be fixed with Exactly Once semantics,
>> >> > see KUDU-1537.
>> >> >
>> >> > This doesn't look like the kind of style change we want, right?
>> >> > Historically we don't annotate our TODOs with names.
>> >> >
>> >> > Or should I reformat it as "TODO(KUDU-1537)..." ?
>> >> >
>> >>
>> >> Yea, I think TODO(bug#) is a good policy to try to have moving forward,
>> but
>> >> I don't think we have to be religious about it. I can turn off this tidy
>> >> check if we think it's not worth it with a codebase of our size.
>> >>
>> >> This guideline comes from Google (
>> >> https://google.github.io/styleguide/cppguide.html#TODO_Comments) where
>> >> they
>> >> say:
>> >>
>> >> TODOs should include the string TODO in all caps, followed by the name,
>> >> e-mail address, bug ID, or other identifier of the person or issue with
>> the
>> >> best context about the problem referenced by the TODO. The main purpose
>> is
>> >> to have a consistent TODO that can be searched to find out how to get
>> more
>> >> details upon request. A TODO is not a commitment that the person
>> referenced
>> >> will fix the problem. Thus when you create a TODO with a name, it is
>> almost
>> >> always your name that is given.
>> >> It's sort of nice to leave a breadcrumb, but it's also not too hard for
>> >> someone to 'git blame' and figure out who added it, so I could go either
>> >> way.
>> >>
>> >> -Todd
>> >>
>> >>
>> >> >
>> >> >
>> >> > On Wed, Sep 14, 2016 at 10:04 PM, Todd Lipcon 
>> wrote:
>> >> > > Hey folks,
>> >> > >
>> >> > > I've set up a jenkins job and gerrit trigger to run clang-tidy-diff
>> on
>> >> > any
>> >> > > patches that are uploaded. It should be set up now so as not to
>> vote +1
>> >> > or
>> >> > > -1, but just to produce comments. For an example of the type of
>> >> warnings
>> >> >

Re: 'tidy-bot' now running (experimentally)

2016-09-16 Thread Adar Dembo
OK, let's keep it then. I changed the comment in question to be
"TODO(KUDU-1537)...".

On Thu, Sep 15, 2016 at 8:18 PM, Jake Farrell  wrote:
> we use "TODO(bug id or committer id): msg" as the format in other projects
> and that seems to be enough breadcrumb in most cases
>
> -Jake
>
> On Thu, Sep 15, 2016 at 11:06 PM, Todd Lipcon  wrote:
>
>> On Thu, Sep 15, 2016 at 6:50 PM, Adar Dembo  wrote:
>>
>> > In https://gerrit.cloudera.org/#/c/4435/, Tidy Bot said:
>> >
>> > Line 209:   // TODO: Should be fixed with Exactly Once semantics,
>> > see KUDU-1537.
>> > warning: missing username/bug in TODO [google-readability-todo]
>> >   // TODO: Should be fixed with Exactly Once semantics, see
>> KUDU-1537.
>> >   ^
>> >   // TODO(unknown): Should be fixed with Exactly Once semantics,
>> > see KUDU-1537.
>> >
>> > This doesn't look like the kind of style change we want, right?
>> > Historically we don't annotate our TODOs with names.
>> >
>> > Or should I reformat it as "TODO(KUDU-1537)..." ?
>> >
>>
>> Yea, I think TODO(bug#) is a good policy to try to have moving forward, but
>> I don't think we have to be religious about it. I can turn off this tidy
>> check if we think it's not worth it with a codebase of our size.
>>
>> This guideline comes from Google (
>> https://google.github.io/styleguide/cppguide.html#TODO_Comments) where
>> they
>> say:
>>
>> TODOs should include the string TODO in all caps, followed by the name,
>> e-mail address, bug ID, or other identifier of the person or issue with the
>> best context about the problem referenced by the TODO. The main purpose is
>> to have a consistent TODO that can be searched to find out how to get more
>> details upon request. A TODO is not a commitment that the person referenced
>> will fix the problem. Thus when you create a TODO with a name, it is almost
>> always your name that is given.
>> It's sort of nice to leave a breadcrumb, but it's also not too hard for
>> someone to 'git blame' and figure out who added it, so I could go either
>> way.
>>
>> -Todd
>>
>>
>> >
>> >
>> > On Wed, Sep 14, 2016 at 10:04 PM, Todd Lipcon  wrote:
>> > > Hey folks,
>> > >
>> > > I've set up a jenkins job and gerrit trigger to run clang-tidy-diff on
>> > any
>> > > patches that are uploaded. It should be set up now so as not to vote +1
>> > or
>> > > -1, but just to produce comments. For an example of the type of
>> warnings
>> > it
>> > > generates, check out:
>> > > https://gerrit.cloudera.org/#/c/4409/4/src/kudu/consensus/
>> > raft_consensus_state.h
>> > >
>> > > If you see any checks that you think are false positives, feel free to
>> > ping
>> > > me and I can either disable those checks entirely, or see if there's
>> some
>> > > configuration we can make to better match our own guidelines.
>> > >
>> > > Hopefully this turns out to be a useful bit of "automatic code review"
>> so
>> > > that we can focus our review efforts less on mechanical checks and more
>> > on
>> > > things requiring human judgment :) If it turns out to be more of an
>> > > annoyance than a help, we can always remove it or really dial back to
>> > only
>> > > the most important warnings.
>> > >
>> > > Also worth noting that these checks are not that complicated to write,
>> so
>> > > if we see that there are some Kudu-specific ones worth implementing, we
>> > can
>> > > easily do so.
>> > >
>> > > -Todd
>> > > --
>> > > Todd Lipcon
>> > > Software Engineer, Cloudera
>> >
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>


Re: 'tidy-bot' now running (experimentally)

2016-09-15 Thread Adar Dembo
In https://gerrit.cloudera.org/#/c/4435/, Tidy Bot said:

Line 209:   // TODO: Should be fixed with Exactly Once semantics,
see KUDU-1537.
warning: missing username/bug in TODO [google-readability-todo]
  // TODO: Should be fixed with Exactly Once semantics, see KUDU-1537.
  ^
  // TODO(unknown): Should be fixed with Exactly Once semantics,
see KUDU-1537.

This doesn't look like the kind of style change we want, right?
Historically we don't annotate our TODOs with names.

Or should I reformat it as "TODO(KUDU-1537)..." ?


On Wed, Sep 14, 2016 at 10:04 PM, Todd Lipcon  wrote:
> Hey folks,
>
> I've set up a jenkins job and gerrit trigger to run clang-tidy-diff on any
> patches that are uploaded. It should be set up now so as not to vote +1 or
> -1, but just to produce comments. For an example of the type of warnings it
> generates, check out:
> https://gerrit.cloudera.org/#/c/4409/4/src/kudu/consensus/raft_consensus_state.h
>
> If you see any checks that you think are false positives, feel free to ping
> me and I can either disable those checks entirely, or see if there's some
> configuration we can make to better match our own guidelines.
>
> Hopefully this turns out to be a useful bit of "automatic code review" so
> that we can focus our review efforts less on mechanical checks and more on
> things requiring human judgment :) If it turns out to be more of an
> annoyance than a help, we can always remove it or really dial back to only
> the most important warnings.
>
> Also worth noting that these checks are not that complicated to write, so
> if we see that there are some Kudu-specific ones worth implementing, we can
> easily do so.
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera


Re: [VOTE] Apache Kudu 1.0.0 RC1

2016-09-13 Thread Adar Dembo
+1

I downloaded the tarball and built it on my Ubuntu Xenial laptop. I
ran the unit tests with ASAN/UBSAN enabled. Didn't see any failures.


On Tue, Sep 13, 2016 at 12:39 AM, Todd Lipcon  wrote:
> Hi,
>
> The Apache Kudu team is happy to announce the first release candidate for
> Apache Kudu 1.0.0.
>
> After approximately a year of beta releases, Apache Kudu has reached
> version 1.0.
> This version number signifies that the development team feels that Kudu is
> stable
> enough for usage in production environments.
>
> The is a source-only release. The artifacts were staged here:
> https://dist.apache.org/repos/dist/dev/kudu/1.0.0-RC1/
>
> Java convenience binaries in the form of a Maven repository are staged here:
> https://repository.apache.org/content/repositories/orgapachekudu-1001/
>
> It was built from this tag:
> https://git-wip-us.apache.org/repos/asf?p=kudu.git;a=commit;h=6f6e49ca98c3e3be7d81f88ab8a0f9173959b191
>
> The release notes can be found here (some links from this document will
> only work when this version is released):
> https://github.com/apache/kudu/blob/branch-1.0.x/docs/release_notes.adoc
>
> KEYS file:
> http://www.apache.org/dist/kudu/KEYS
>
> I'd suggest going through the README, building Kudu, and running the
> unit tests. Testing out the Maven repo would also be appreciated.
>
> Please try the release and vote; the vote will end in 72 hours (on
> 9/16/2016 at 12:30am PDT).
>
> Thanks,
> Todd


Re: [ANNOUNCE] Two new Kudu committer/PMC members

2016-09-12 Thread Adar Dembo
Congrats guys!


On Mon, Sep 12, 2016 at 3:55 PM, Todd Lipcon  wrote:
> Hi Kudu community,
>
> It's my great pleasure to announce that the PMC has voted to add both
> Alexey Serbin and Will Berkeley as committers and PMC members.
>
> Alexey has been contributing for a few months, including developing some
> pretty meaty (and tricky) additions. Two of note are the addition of
> doxygen for our client APIs, as well as the implementation of
> AUTO_FLUSH_BACKGROUND in C++. He has also been quite active in reviews
> recently, having reviewed 40+ patches in the last couple months. He also
> contributed by testing and voting on the recent 0.10 release.
>
> Will has been a great contributor as well, spending a lot of time in areas
> that don't get as much love from others. In particular, he's made several
> fixes to the web UIs, has greatly improved the Flume integration, and has
> been burning down a lot of long-standing bugs recently. Will also spends a
> lot of his time outside of Kudu working with users and always has good
> input on what our user community will think of a feature. Like Alexey, Will
> also participated in the 0.10 release process.
>
> Both of these community members have already been "acting the part" through
> their contributions detailed above, and the PMC is excited to continue
> working with them in their expanded roles.
>
> Please join me in congratulating them!
>
> -Todd


Re: RFC: rename "remote bootstrap" to "tablet copy"?

2016-08-01 Thread Adar Dembo
I don't have much of an opinion one way or the other. I'm not put off
by the current name but wouldn't object to changing it either,
provided you're signing up for the work. :)

Tablet copy seems like a reasonable name to me.

On Sun, Jul 31, 2016 at 5:13 PM, Alexey Serbin  wrote:
> Todd,
>
> Change from 'remote bootstrap' to something like 'tablet copy' sounds
> reasonable.  As I see from the description, the 'tablet copy' better
> reflects the essence of the process.  Both 'tablet copy' and 'tablet
> snapshot transfer' are better than 'remote bootstrap', IMO.  Not sure
> whether the additional 'remote' brings more clarity: is it possible to have
> a 'local tablet copy' at all?
>
> BTW, if 'replica' or 'replication' terms aren't too overloaded and
> applicable in this context, consider a couple of additional options:
> - Tablet replication
> - Creating tablet replica
>
>
> Best regards,
>
> Alexey
>
> On Sun, Jul 31, 2016 at 3:45 PM, Todd Lipcon  wrote:
>
>> Hi all,
>>
>> Currently we use the term "remote bootstrap" to mean the process by which
>> one tablet server copies a tablet from another. This terminology started
>> long long ago, back when we thought there was some chance that it would
>> actually _start_ the tablet from remote storage, which isn't at all what
>> the current design does.
>>
>> I think it would be more accurate and cause less confusion if we considered
>> changing our terminology here. The current terminology invites confusion
>> with "tablet bootstrap", the process by which a tablet opens its data,
>> replays its logs, and starts. A couple suggestions would be:
>>
>> - Tablet copy (since we're copying a tablet from one host to another)
>> - Tablet snapshot transfer (a mouthful,but fairly accurate)
>> - Remote tablet copy (slightly more precise than "tablet copy" but also
>> longer)
>>
>> What do people think? Changing this before we hit 1.0 would be nice just so
>> we have time to update docs, metric names, etc, even though it doesn't
>> affect any public APIs.
>>
>> -Todd
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>