date:20181212

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11961 )

Change subject: De-flake sentry tests
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11961/4/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:

http://gerrit.cloudera.org:8080/#/c/11961/4/src/kudu/util/net/net_util.cc@451
PS4, Line 451:   static const int kServerIdxBits = 24 - kPidBits;
You can reuse this constant in kServersMaxNum too.


http://gerrit.cloudera.org:8080/#/c/11961/4/src/kudu/util/net/net_util.cc@452
PS4, Line 452:   CHECK(0 < index && index <= kServersMaxNum) << Substitute(
I think I missed something: why did the requirements of 'index' change such 
that 0 < index <= max_servers instead of 0 <= index < max_servers? The former 
is more intuitive; it's more natural to see intervals that are closed on the 
lower bound and open on the upper bound.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b
Gerrit-Change-Number: 11961
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 13 Dec 2018 05:38:23 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2636: LBM supports deleting dead and full containers

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12075 )

Change subject: KUDU-2636: LBM supports deleting dead and full containers
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1179
PS1, Line 1179:
  : ///
  : // LogBlockCreationTransaction
  : 
> Agree with point 1 and 3.
Point 2 is spelled out in more detail in Repair(). I'll copy the relevant 
comment:

  // Delete all dead containers.
  //
  // After the deletions, the data directory is sync'ed to reduce the chance
  // of a data file existing without its corresponding metadata file (or vice
  // versa) in the event of a crash. The block manager would treat such a case
  // as corruption and require manual intervention.
  //
  // TODO(adar) the above is not fool-proof; a crash could manifest in between
  // any pair of deletions. That said, the odds of it happening are incredibly
  // rare, and manual resolution isn't hard (just delete the existing file).

To go into more detail, the concern here is that once we start deleting files, 
a machine crash could lead to any subset of the deleted files coming back to 
life, regardless of the order in which they were deleted. Without a SyncDir() 
call, this vulnerability window is as long as the filesystem's journal commit 
period. On ext4 that's given by the 'commit' mount option, which defaults to 5s.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10
Gerrit-Change-Number: 12075
Gerrit-PatchSet: 2
Gerrit-Owner: helifu 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu 
Gerrit-Comment-Date: Thu, 13 Dec 2018 05:24:21 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2636: LBM supports deleting dead and full containers

2018-12-12 Thread helifu (Code Review)

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: KUDU-2636: LBM supports deleting dead and full containers
..

KUDU-2636: LBM supports deleting dead and full containers

LBM supports deleting the full container which is dead
after hole punching. It can help to save startup time.
It uses the file's physical size to determine whether
the container is dead or live.

Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 136 insertions(+), 15 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10
Gerrit-Change-Number: 12075
Gerrit-PatchSet: 2
Gerrit-Owner: helifu 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu

[kudu-CR] KUDU-2636: LBM supports deleting dead and full containers

2018-12-12 Thread helifu (Code Review)

helifu has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12075 )

Change subject: KUDU-2636: LBM supports deleting dead and full containers
..


Patch Set 1:

(3 comments)

Thanks for your comments.@adar

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1179
PS1, Line 1179:   // Though the result of deleting a data block is not the same 
as
  :   // before when there are empty data blocks in the same 
container
  :   // which is FULL, especially in the unit test, this design is 
still
  :   // acceptable.
> I don't agree with this assessment. This approach suffers from the followin
Agree with point 1 and 3.
But to point 2, i have a question, is it necessary to call SyncDir() after 
deleting the container? In my opinion it's ok that let the file system flush 
dirty data to disk.Then, the 'batch' containers deletion is unnecessary.


http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h@648
PS1, Line 648:   virtual Status SizeOnDisk(uint64_t* size) const = 0;
> Could you add a brief unit test to env-test to cover this new method?
rollback.


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

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env_posix.cc@1299
PS1, Line 1299:   // From stat(2):
  :   //
  :   //   The st_blocks field indicates the number of blocks 
allocated to
  :   //   the file, 512-byte units. (This may be smaller than 
st_size/512
  :   //   when the file has holes.)
  :   *size = sbuf.st_blocks * 512;
> Can you reuse this block of code for RWFile::SizeOnDisk, using a decomposed
rollback.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10
Gerrit-Change-Number: 12075
Gerrit-PatchSet: 1
Gerrit-Owner: helifu 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu 
Gerrit-Comment-Date: Thu, 13 Dec 2018 03:40:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-683: unify C++ client-to-master retry logic

2018-12-12 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12002 )

Change subject: KUDU-683: unify C++ client-to-master retry logic
..


Patch Set 5:

(6 comments)

just nits at this point from me

http://gerrit.cloudera.org:8080/#/c/12002/2/src/kudu/client/master_proxy_rpc.h
File src/kudu/client/master_proxy_rpc.h:

http://gerrit.cloudera.org:8080/#/c/12002/2/src/kudu/client/master_proxy_rpc.h@58
PS2, Line 58:  
nit: here and below for the 'virtual' and 'override': leave just 'override' 
since those are overrides and that implies 'virtual'.


http://gerrit.cloudera.org:8080/#/c/12002/2/src/kudu/client/master_proxy_rpc.h@100
PS2, Line 100:
 :   // With a new leader found, resends the RPC. 'creds_policy' is 
the policy
 :   // with which the reconnection was attempted.
 :   virtual void NewLeaderMasterDeterminedCb(rpc::CredentialsPolicy
nit: is it possible to use std::function here instead?  Not that it's crucial, 
but since you are modifying this code in some non-trivial manner anyways, it 
might make sense to convert boost::bind() and boost::function to their STL 
counterparts.


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc
File src/kudu/client/master_proxy_rpc.cc:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@196
PS5, Line 196: this->mutable_retrier(
readability nit: here and below, maybe store the pointer to the retrier into a 
local variable and reuse it in the code of this method?


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@217
PS5, Line 217: client_->IsMultiMaster()
readability nit: since the multi-master property is a not dynamic one, maybe 
store it into a boolean variable and use it here and below


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@20
PS5, Line 20: stdint.h
nit:  ?


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@563
PS5, Line 563: virtual
nit here and below for virtual methods marked with 'override': drop 'virtual'



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2450676da1c723a247c84deb1b895f116173670e
Gerrit-Change-Number: 12002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 03:38:50 +
Gerrit-HasComments: Yes

[kudu-CR] De-flake sentry tests

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11961 )

Change subject: De-flake sentry tests
..


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/mini-cluster/external_mini_cluster.cc@525
PS3, Line 525: string ExternalMiniCluster::GetBindIpForExternalServer(int 
index) const {
> Nit: indentation.
Done


http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/sentry/sentry-test-base.h@44
PS3, Line 44: std::string host = GetBindIpForDaemon(1, kDefaultBindMode);
: HostPort address(host, 0);
: sentry_->SetAddress(address);
: if (kerberos_enabled_) {
:   
> Shouldn't this use kDefaultBindMode?
Done


http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h@165
PS3, Line 165:
> Given that ExternalMiniCluster isn't test code anymore, I don't think this
Done


http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h@206
PS3, Line 206:
> We're kind of far away from daemons at this point. Maybe discuss what guara
Done


http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.cc@514
PS3, Line 514:
> nit: spacing
Discussed with Andrew offline. I think this is a false alarm.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b
Gerrit-Change-Number: 11961
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 13 Dec 2018 02:49:10 +
Gerrit-HasComments: Yes

[kudu-CR] De-flake sentry tests

Hello Kudu Jenkins, Andrew Wong, Adar Dembo,

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

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

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

Change subject: De-flake sentry tests
..

De-flake sentry tests

Currently, sentry_client-test and sentry_authz_provider-test are still
flaky due to possible port collision. To avoid such race condition, this
patch moves the util function that finds the bind address for a daemon
based on binding mode to test_util, and updates sentry-test-base to use
it to pick up a unique bind address.

Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b
---
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/mini-cluster/mini_cluster.h
M src/kudu/sentry/sentry-test-base.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
13 files changed, 163 insertions(+), 139 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b
Gerrit-Change-Number: 11961
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11956 )

Change subject: [sentry] add mini Sentry to the external mini cluster
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99:   std::string ip_{"0.0.0.0"};
> Sorry for the confusion; my comment was just surprise that "Foo f_ = ..." s
Thanks for the explanation. Yes, this is legal.

Referred §12.6.2/8 of C++11 draft 
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf):
In a non-delegating constructor, if a given non-static data member or base 
class is not designated by a mem-initializer-id (including the case
where there is no mem-initializer-list because the constructor has no 
ctor-initializer) and the entity is not a virtual base class of an abstract 
class (10.4), then if the entity is a non-static data member that has a 
brace-or-equal-initializer, the entity is initialized as specified in 8.5;

For narrowing conversion, I agree this is not related to the usage here.

I will switch it back.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 02:24:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-683: unify C++ client-to-master retry logic

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12002 )

Change subject: KUDU-683: unify C++ client-to-master retry logic
..


Patch Set 5:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/12002/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12002/5//COMMIT_MSG@27
PS5, Line 27: - SyncLeaderMasterRpcs used exponential backoff when retrying 
RPCs, but
:   the existing RpcRetrier implemented its own backoff. To maintain
:   this, I've extended RpcRetrier with an ExponentialRpcRetrier 
class
:   that computes an exponential backoff. The RpcRetrier type is a
:   template parameter to the AsyncLeaderMasterRpc.
I'm not convinced that both kinds of backoff are warranted, but let's pretend 
that they are: could you implement this without inheritance? Perhaps as an enum 
passed as to the RpcRetrier constructor? Or an enum used as a template 
parameter?


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h
File src/kudu/client/master_proxy_rpc.h:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@51
PS5, Line 51: class AsyncLeaderMasterRpc : public rpc::RetryingRpc {
Doc the class too.


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@53
PS5, Line 53:   AsyncLeaderMasterRpc(const MonoTime& deadline,
Lots of args, please doc.


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@55
PS5, Line 55:const ReqClass& req,
:RespClass* resp,
Can the AsyncLeaderMasterRpc declare its own req/resp members and provide 
accessors to them? Or is it important to be able to provide them as args here?


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc
File src/kudu/client/master_proxy_rpc.cc:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@281
PS5, Line 281: template
For consistency, can you format all of these the same way?


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/retriable_rpc.h@51
PS5, Line 51: class RetriableRpc : public RetryingRpc {
Hard to reason about RetriableRpc vis a vis RetryingRpc.


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h
File src/kudu/rpc/rpc.h:

http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h@115
PS5, Line 115: class RpcRetrier {
Could we merge the retrier into the Rpc class? I think the decomposition has 
been a net loss: the retrier isn't particular complex, but there's a great deal 
of plumbing in it (and in its consumers) to facilitate the exchange across 
class boundaries.

In particular I'm not so sure HandleResponse() has worked out; the depth of 
these class hierarchies leads to RPC "handling" at multiple levels that's hard 
to follow.


http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h@229
PS5, Line 229: // An RPC that gets retried based on some retrying logic.
It's tough to see the distinction between this class and Rpc. Both expose a 
retrier/mutable_retrier, and although Rpc doesn't mandate where it comes from, 
where else would it come from but a class member? Likewise, the distinction 
between the base class' stipulation about num_attempts and the override seem 
almost immaterial.

If the goal here is to provide a pure interface and a partial implementation, I 
don't think that's warranted in C++ where partial interfaces are a thing (by 
contrast, Java requires interfaces to be pure). I think it'd be OK for Rpc to 
provide some base implementations to avoid the extra class in the hierarchy.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2450676da1c723a247c84deb1b895f116173670e
Gerrit-Change-Number: 12002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 01:00:59 +
Gerrit-HasComments: Yes

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11956 )

Change subject: [sentry] add mini Sentry to the external mini cluster
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99:   std::string ip_{"0.0.0.0"};
> After accommodated your comment I thought list initialization is more commo
Sorry for the confusion; my comment was just surprise that "Foo f_ = ..." style 
member initialization was legal. If it is, great. I think it's more 
approachable than "Foo f_{ ... }" so all else being equal I'd revert this back 
to how it was before.

What did you mean by not allowing narrowing conversion? We're not dealing with 
integer types here, so I'm not seeing the connection.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 01:05:21 +
Gerrit-HasComments: Yes

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11956 )

Change subject: [sentry] add mini Sentry to the external mini cluster
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc@269
PS9, Line 269: // Restart Sentry with the HMS address.
> Even though we can't configure HMS's bind address, I think we can still avo
Yeah, I think it would be a bit messy, but I agree the gain worth the try.


http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99:   std::string ip_{"0.0.0.0"};
> Now I'm confused; why did you switch from copy init to value init? Copy ini
After accommodated your comment I thought list initialization is more common in 
this case and preferred as it does not allow narrowing conversion. Maybe I 
misunderstand your comment?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 00:49:05 +
Gerrit-HasComments: Yes

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )

Change subject: [sentry] add mini Sentry to the external mini cluster
..

Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257
PS2, Line 257: hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"],
spn, ktpath,
> Hmm, it looks like so. Although all of our work are based Hive 2.3.x curren
Yeah, and looks like Hive 4 isn't even released yet. Making our own fork of
Hive 2.3.x to apply this one patch is probably too much work, so I guess we'll
have to live with this.

When you merge, could you file a Kudu JIRA to track the progression of this
Hive feature, so that when we do upgrade to a version of Hive that supports it,
we'll be able to simplify this code?

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc@269
PS9, Line 269: // Restart Sentry with the HMS address.
Even though we can't configure HMS's bind address, I think we can still avoid a
restart:

1. Generate a UNIQUE_LOOPBACK address for Sentry.
2. Statically choose a non-zero port that Sentry will use. Since Sentry will
live on its own UNIQUE_LOOPBACK, there's no danger of collision with anything
else and you could pick whatever port number you wanted.
3. Start HMS configured to talk to Sentry at the address you got in #1 and port
you got in #2.
4. Start Sentry configured at the address/port from #1/#2, and configured to
talk to the HMS from #3.

Granted, this only works well for UNIQUE_LOOPBACK, so macOS will need to do the
restart thing. Would the code get too messy with a UNIQUE_LOOPBACK case and a
non-UNIQUE_LOOPBACK case? FWIW I'd have a pretty high tolerance for messy code
in this case because the payoff is so significant (faster test run time on
Linux).

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:

http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99: std::string ip_{"0.0.0.0"};
Now I'm confused; why did you switch from copy init to value init? Copy init is
more familiar to most people.

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65
PS8, Line 65: // Whether or not the Sentry service should be set up.
> Would you mind I do this in a follow up patch? This reminds me to add a tes
Sure, please revert this hunk then.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 00:03:23 +
Gerrit-HasComments: Yes

[kudu-CR] [Java] Add a Schema and Data Generator

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/12061 )

Change subject: [Java] Add a Schema and Data Generator
..

[Java] Add a Schema and Data Generator

This patch adds schema and data generator utility
classes that can be used to create random tables and
random data. These utilities are useful in fuzz tests
and for various load and scale test applications.

The initial implementation is inteneded to be
fairly flexble without being overengineered.
Follow on patches will improve the API and options.

The classes are currently marked private, but could be
changed in the future.

Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Reviewed-on: http://gerrit.cloudera.org:8080/12061
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
A java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java
4 files changed, 622 insertions(+), 152 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Gerrit-Change-Number: 12061
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [Java] Add a Schema and Data Generator

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12061 )

Change subject: [Java] Add a Schema and Data Generator
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Gerrit-Change-Number: 12061
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Dec 2018 23:43:58 +
Gerrit-HasComments: No

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11956 )

Change subject: [sentry] add mini Sentry to the external mini cluster
..


Patch Set 9:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:

http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322
PS2, Line 322: Configures the HMS to use the Sentry post-event listener, which
 : // synchronizes the HMS events with t
> Ah, I think my confusion is in separating out the rules for the HMS vs the
Discussed with Andrew offline. Updated it based on his suggestion. Although 
note that config is not specific to Kudu instead is for entire HMS operations.


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79
PS8, Line 79: sentry_service_principal
> nit: perhaps DCHECK this isn't empty?
Done


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229
PS8, Line 229:
 :   static const string kHiveFileTemplate = R"(
 :  nit: should be a part of kHiveSentryFileTemplate?
Done


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310
PS8, Line 310:
 : // - hive.metastore.filter.hook
 : // Configures the HMS to use the Sentry plugin for 
filtering
 : // out information user has no priv
> Does this mean that we'll only see HMS notifications that the `kudu` user h
This is only for SHOWTABLES and SHOWDATABASES.


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321
PS8, Line 321: ve.metastore.event.listeners
 : // Configures the HMS to use the Sentry post-event 
listener, which
 : // synchronizes the HMS events with the Sentry service. 
The Sentry
 : // service will be made aware of events like table 
renames and
 : // update itself accor
> nit reword a bit:
It is not. That is for HDFS. Reword based on your recommendation, although it 
is synchronizing the HMS events with the Sentry service not the other way 
around.


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@46
PS8, Line 46: using std::string;
> warning: using decl 'ExternalMiniCluster' is unused [misc-unused-using-decl
Done


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@51
PS8, Line 51: // integration enabled.
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95
PS8, Line 95:
> nit: kTableIdentifier
Done


http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257
PS2, Line 257:   hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], 
spn, ktpath,
> Search for THRIFT_BIND_HOST in HIVE-20794. Does that address HIVE-18998? It
Hmm, it looks like so. Although all of our work are based Hive 2.3.x currently.


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231
PS8, Line 231: 2. Start the HMS, configured to talk to the Sentry service. Find 
out
 :   //which port it's on
> It's been a while since we talked about it, but is it not possible that ano
Done


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64
PS8, Line 64: case EXTERNAL_SERVER:
:   idx = kServersMaxNum / 3 + index;
> Somewhat of a nit: this is giving the larger partition to the masters, when
Given the kServerMaxNum is 62, that means TSERVER can range from [1,20) I think 
this is enough for our test cases. But I can switch the calculation between 
master and tserver.


http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:

http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114
PS8, Line 114: 127.0.0.1
> Why doesn't this need to be the HMS URIs?
This 'hack' works, because we don't use other IP address for the HMS. And I 
would like to keep it this way as it is simple. And we can always change it if 
it no longer applies.


http://gerrit.clo

[kudu-CR] [sentry] add mini Sentry to the external mini cluster

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo,

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

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

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

Change subject: [sentry] add mini Sentry to the external mini cluster
..

[sentry] add mini Sentry to the external mini cluster

This commit enables external mini cluster to be able to start with both
mini sentry and mini hms. A major challenge is the circular dependency
between the HMS and Sentry in terms of configuring each with the other's
address, including port which is currently discovered by polling lsof.
To work around it, one approach would be to:

1. Start the Sentry service. Find out which port it's on. At this
stage, the Sentry service has no knowledge of any HMS service.
2. Start the HMS, configured to talk to Sentry's port. Find out which
port it's on.
3. Restart the Sentry service on the same port, reconfigured to talk
to the HMS's port.

However, there could be a race condition where another program binds to
the same port during the restart in step 3. One option is to use
SO_REUSEPORT socket option, which permits multiple sockets to be bound
to an identical socket address. Although both Sentry and HMS are written
in Java, only JDK9 (and above) supports this socket option[1]. Methods
such as Java reflection can be used to invoke private methods to set up
this option, but they are hacky.

This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that
external servers such as Sentry can pick up a unique bind address,
knowing that in doing so there'll never be any danger of a port
collision.

[1]:
https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT

Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
---
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/mini-cluster/mini_cluster.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/sentry/mini_sentry.cc
M src/kudu/sentry/mini_sentry.h
M src/kudu/sentry/sentry-test-base.h
M src/kudu/tools/tool.proto
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
16 files changed, 516 insertions(+), 69 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/9
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [Java] Add a Schema and Data Generator

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12061 )

Change subject: [Java] Add a Schema and Data Generator
..


Patch Set 1:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/12061/1//COMMIT_MSG@9
PS1, Line 9: This patch adds a schema and data generator utility
   : class
> Nit: This patch adds schema and data generate utility classes
Done


http://gerrit.cloudera.org:8080/#/c/12061/1//COMMIT_MSG@11
PS1, Line 11: usefull
> useful
Done


http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
File java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java:

PS1:
> License header.
Done


http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@58
PS1, Line 58:   public void randomizeRow(PartialRow row) {
> Seems redundant to have both randomizeRow and randomRow. The TestKuduBackup
Done


http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@97
PS1, Line 97:   i++;
> It's easy to miss this; perhaps convert into a for loop on i (capped at col
Done


http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@129
PS1, Line 129:   public static class DataGeneratorBuilder {
> Since the only way to create a DataGenerator is through here, maybe doc thi
Done


http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java
File java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java:

PS1:
> License header.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Gerrit-Change-Number: 12061
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Dec 2018 22:09:10 +
Gerrit-HasComments: Yes

[kudu-CR] [Java] Upgrade dependencies

Grant Henke has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/12076 )

Change subject: [Java] Upgrade dependencies
..

[Java] Upgrade dependencies

Upgrades the Java dependencies and Gradle versions.

This patch also has a few build fixes to support
Gradle 5.0.

Minor version upgrades:
- ClojureToolsCli 0.3.5 -> 0.4.1
- Guava 26.0-android -> 27.0-android
- Mockito 2.22.0 -> 2.23.4

Maintenance version upgrades:
- Errorprone 2.3.1 -> 2.3.2
- Hive 2.3.3 -> 2.3.4

Gradle upgrades:
- Gradle 4.10.2 -> 5.0
- gradle-avro-plugin 0.15.1 -> 0.16.0
- shadow 2.0.4 -> 4.0.2
- protobuf-gradle-plugin 0.8.6 -> 0.8.7
- nebula-clojure-plugin 6.0.2 -> 7.0.1
- spotbugs-gradle-plugin 1.6.4 -> 1.6.5

Change-Id: Ic09189d7fee7f2348718375083c32fa7b00ec5c0
Reviewed-on: http://gerrit.cloudera.org:8080/12076
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/buildSrc/build.gradle
M java/gradle/compile.gradle
M java/gradle/dependencies.gradle
M java/gradle/docs.gradle
M java/gradle/shadow.gradle
M java/gradle/wrapper/gradle-wrapper.properties
M java/gradlew
7 files changed, 26 insertions(+), 29 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic09189d7fee7f2348718375083c32fa7b00ec5c0
Gerrit-Change-Number: 12076
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [Java] Add a Schema and Data Generator

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: [Java] Add a Schema and Data Generator
..

[Java] Add a Schema and Data Generator

This patch adds schema and data generator utility
classes that can be used to create random tables and
random data. These utilities are useful in fuzz tests
and for various load and scale test applications.

The initial implementation is inteneded to be
fairly flexble without being overengineered.
Follow on patches will improve the API and options.

The classes are currently marked private, but could be
changed in the future.

Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
---
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
A java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java
4 files changed, 622 insertions(+), 152 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Gerrit-Change-Number: 12061
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [Java] Upgrade dependencies

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12076 )

Change subject: [Java] Upgrade dependencies
..


Patch Set 1: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/12076/1//COMMIT_MSG@24
PS1, Line 24: - Gradle 4.10.2 -> 5.0
https://github.com/gradle/gradle/issues/4629 was fixed. Woo!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic09189d7fee7f2348718375083c32fa7b00ec5c0
Gerrit-Change-Number: 12076
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Dec 2018 21:40:06 +
Gerrit-HasComments: Yes

[kudu-CR] De-flake sentry tests

2018-12-12 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11961 )

Change subject: De-flake sentry tests
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h@206
PS3, Line 206: // Return the IP address that the daemon with the given index 
will bind to.
We're kind of far away from daemons at this point. Maybe discuss what 
guarantees we can expect based on the index? E.g. what happens when you call 
this twice with the same index vs with different indexes?


http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:

http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.cc@514
PS3, Line 514: "last I
nit: spacing



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b
Gerrit-Change-Number: 11961
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Dec 2018 21:06:02 +
Gerrit-HasComments: Yes

[kudu-CR] java: add log4j.properties file to kudu-test-utils module

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12040 )

Change subject: java: add log4j.properties file to kudu-test-utils module
..


Patch Set 3:

I am okay to have a single change adding the missing log4j.properties and 
optimizing/standardizing in a follow up.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic57941d593c32d17c9619f51d18430a75e962635
Gerrit-Change-Number: 12040
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Wed, 12 Dec 2018 21:02:00 +
Gerrit-HasComments: No

[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager

2018-12-12 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11797 )

Change subject: [sentry] Integrate AuthzProvider into CatalogManager
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/catalog_manager.cc@1405
PS2, Line 1405: Skip authorization validation if a
  :   // remote call context is not provided, which implies there 
is no remote
  :   // calls.
Does this ever happen? If it's not a case we expect, maybe this should be a 
D/CHECK(rpc) instead.


http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1710
PS3, Line 1710: return 
SetupError(authz_provider_->AuthorizeGetTableMetadata(username, table_name),
  :   resp, MasterErrorPB::NOT_AUTHORIZED);
Hrm, WDYT about baking IsTrustedUser into these AuthzProvider::Authorize* 
calls? Then we don't have to do that check in the catalog manager code, eg at 
L1411, L1770, etc. Also we'd be able to avoid these calls entirely in case the 
DefaultAuthzProvider is being used.


http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1771
PS3, Line 1771: authz_func(*user, table_name);
I'm a little surprised we're using the authz function to authorize locking the 
table. Why doesn't it need to be METADATA ON DATABASE or something?


http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1778
PS3, Line 1778: NOT_AUTHOIRZED
nit: NOT_AUTHORIZED


http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/master.proto@85
PS3, Line 85: // The caller is not authorized to perform the attempted 
operation.
> In your mind, how is this different from Status::NotAuthorized? If I issue
It's worth noting what we do for scanner-hijacking (see SetupErrorAndRespond() 
tserver/tablet_service.cc). Within the context of the tserver, we pass around a 
TabletServerErrorPB::NOT_AUTHORIZED that indicates that the user was not 
authorized to make the request and that the RPC response should reflect that. 
It does so by returning FATAL_UNAUTHORIZED as the RPC error, and it attaches a 
Status that has more context. Client-side, an RPC error results in a 
RemoteError, and the attached Status is just a part of the Status' body. For 
scanner-hijacking, this Status is NotAuthorized, though it doesn't necessarily 
need to be -- it's just there to add information. The TabletServerErrorPB 
exists solely to decouple tserver error (NOT_AUTHORIZED) and RPC error 
(FATAL_UNAUTHORIZED).

RemoteError is also what the client would see for coarse-grained authorization 
errors (see ServerBase::Authorized() and how it's setting the RPC as a failure).

IIUC, this is taking a different approach: it returns that the RPC was 
successful, transforming a Status::NotAuthorized error into an AppStatusPB  on 
the master (see SetupError() in master/catalog_manager.cc), and the client 
unwraps and gets the original Status::NotAuthorized error. This is happening by 
virtue of Status::NotAuthorized being returned from the catalog manager, not 
the NOT_AUTHORIZED codes.

If we wanted to make this symmetric with the tserver, we could probably do 
something similar: have the catalog manager set NOT_AUTHORIZED, and have the 
master service check for NOT_AUTHORIZED codes and return FATAL_UNAUTHORIZED. I 
can kind of see the merit of having it be an application error though; the 
request itself makes it well past the RPC layer, which makes it a little 
strange that we're responding with remote errors. WDYT?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8
Gerrit-Change-Number: 11797
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 12 Dec 2018 20:54:32 +
Gerrit-HasComments: Yes

[kudu-CR] [Java] Upgrade dependencies

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12076


Change subject: [Java] Upgrade dependencies
..

[Java] Upgrade dependencies

Upgrades the Java dependencies and Gradle versions.

This patch also has a few build fixes to support
Gradle 5.0.

Minor version upgrades:
- ClojureToolsCli 0.3.5 -> 0.4.1
- Guava 26.0-android -> 27.0-android
- Mockito 2.22.0 -> 2.23.4

Maintenance version upgrades:
- Errorprone 2.3.1 -> 2.3.2
- Hive 2.3.3 -> 2.3.4

Gradle upgrades:
- Gradle 4.10.2 -> 5.0
- gradle-avro-plugin 0.15.1 -> 0.16.0
- shadow 2.0.4 -> 4.0.2
- protobuf-gradle-plugin 0.8.6 -> 0.8.7
- nebula-clojure-plugin 6.0.2 -> 7.0.1
- spotbugs-gradle-plugin 1.6.4 -> 1.6.5

Change-Id: Ic09189d7fee7f2348718375083c32fa7b00ec5c0
---
M java/buildSrc/build.gradle
M java/gradle/compile.gradle
M java/gradle/dependencies.gradle
M java/gradle/docs.gradle
M java/gradle/shadow.gradle
M java/gradle/wrapper/gradle-wrapper.properties
M java/gradlew
7 files changed, 26 insertions(+), 29 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic09189d7fee7f2348718375083c32fa7b00ec5c0
Gerrit-Change-Number: 12076
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke

[kudu-CR] KUDU-2636: LBM supports deleting dead and full containers

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12075 )

Change subject: KUDU-2636: LBM supports deleting dead and full containers
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1179
PS1, Line 1179:   // Though the result of deleting a data block is not the same 
as
  :   // before when there are empty data blocks in the same 
container
  :   // which is FULL, especially in the unit test, this design is 
still
  :   // acceptable.
I don't agree with this assessment. This approach suffers from the following 
problems:
1. It goes to disk unnecessarily to figure out if a container is dead. If we 
can safely call full(), why can't we call live_blocks() and compare it to 0?
2. By chaining to ContainerDeletionAsync, we can't "batch" container deletion. 
LogBlockDeletionTransaction allows for the client to specify a group of blocks 
to be deleted together (i.e. deleting an entire tablet). The LBM uses this to 
optimize deletion where it can. In the case of deleting containers that are no 
longer necessary, we can delete all of the container files, then do one 
SyncDir() call per parent directories, instead of a SyncDir() per container.
3. Furthermore, if a LogBlockDeletionTransaction is going to delete all of the 
blocks in a container, it'd be cheaper to recognize that and, rather than 
punching any holes, delete the container files outright.


http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1966
PS1, Line 1966: void LogBlockManager::RemoveDeadFullContainer(const 
std::string& container_name) {
Would be good to reuse/consolidate the file deletion code here with the 
equivalent in Repair().


http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h@648
PS1, Line 648:   virtual Status SizeOnDisk(uint64_t* size) const = 0;
Could you add a brief unit test to env-test to cover this new method?


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

http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env_posix.cc@1299
PS1, Line 1299:   // From stat(2):
  :   //
  :   //   The st_blocks field indicates the number of blocks 
allocated to
  :   //   the file, 512-byte units. (This may be smaller than 
st_size/512
  :   //   when the file has holes.)
  :   *size = sbuf.st_blocks * 512;
Can you reuse this block of code for RWFile::SizeOnDisk, using a decomposed 
static helper? The motivation here is understandability: it'd be good for 
readers to see that comment.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10
Gerrit-Change-Number: 12075
Gerrit-PatchSet: 1
Gerrit-Owner: helifu 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Dec 2018 19:09:53 +
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Update libv8 version in Gemfile.lock

2018-12-12 Thread Attila Bukor (Code Review)

Attila Bukor has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12072 )

Change subject: Update libv8 version in Gemfile.lock
..


Patch Set 1:

> Patch Set 1:
>
> Would be good to get some verification that this works on older versions.

Do you know anyone still on High Sierra? Can you check if it works for you as 
well just in case? Someone on Linux should also verify this works


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I19ed9d0a9548aa2f33b3dde701a9519a717631ee
Gerrit-Change-Number: 12072
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 12 Dec 2018 15:02:19 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Update libv8 version in Gemfile.lock