[kudu-CR] util: pull Random methods out from tests

2019-04-04 Thread Andrew Wong (Code Review) 
Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..

util: pull Random methods out from tests

I've found a couple of Random utility methods pretty useful in some
tests I'm writing, so I'm pulling them out into random_util.

Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Reviewed-on: http://gerrit.cloudera.org:8080/12918
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/master/placement_policy.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/random-test.cc
M src/kudu/util/random.h
M src/kudu/util/random_util.h
5 files changed, 80 insertions(+), 78 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75:  Rand* r, std::vector* result) {
> Mind if I don't? I'd be fighting the temptation to do the same for both Ran
Fair enough.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 04 Apr 2019 01:05:50 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75:  Rand* r, std::vector* result) {
> Nit: could we put 'r' first in the list? Because this is "almost" a Random
Mind if I don't? I'd be fighting the temptation to do the same for both 
RandomString()s.

GSG is a bit fuzzy here since this isn't quite input-only insofar as it is 
mutated.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 04 Apr 2019 01:01:34 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:

http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75:  Rand* r, std::vector* result) {
Nit: could we put 'r' first in the list? Because this is "almost" a Random 
method, I think it feels more natural for the PRNG to be the first argument 
keyed in.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 04 Apr 2019 00:37:25 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 5:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149: std::lock_guard l(lock_);
> But isn't it sufficient to hold the lock around the call to Uniform() withi
Ah I think you're right. Done.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 03 Apr 2019 23:41:34 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: util: pull Random methods out from tests
..

util: pull Random methods out from tests

I've found a couple of Random utility methods pretty useful in some
tests I'm writing, so I'm pulling them out into random_util.

Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
---
M src/kudu/master/placement_policy.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/random-test.cc
M src/kudu/util/random.h
M src/kudu/util/random_util.h
5 files changed, 80 insertions(+), 78 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149:
> The trouble with ThreadSafeRandom.ReservoirSample as a template is that it'
But isn't it sufficient to hold the lock around the call to Uniform() within 
ReservoirSample rather than for the entirety of the method?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 03 Apr 2019 23:23:48 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 4:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149:
> I wonder if we should move these methods as well as the other "utility" met
The trouble with ThreadSafeRandom.ReservoirSample as a template is that it's 
not really doing the exact same thing as Random.ReservoirSample on account of 
it taking a lock. That said, the utilities based on top of it are, so those are 
fair game.


http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@152
PS3, Line 152:  public:
> Maybe rename these a bit to emphasize their similarity? Like, OneFromContai
Went with SelectRandomSubset and SelectRandomElement.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 03 Apr 2019 23:19:39 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: util: pull Random methods out from tests
..

util: pull Random methods out from tests

I've found a couple of Random utility methods pretty useful in some
tests I'm writing, so I'm pulling them out into random_util.

Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
---
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/random_util.h
2 files changed, 38 insertions(+), 32 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149:
> Could you add ThreadSafeRandom equivalents too?
I wonder if we should move these methods as well as the other "utility" methods 
like ReservoirSample into random_util.h instead? We could templatize them on 
the RNG and avoid having to duplicate all the methods.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 03 Apr 2019 17:44:48 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12918 )

Change subject: util: pull Random methods out from tests
..


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149:
Could you add ThreadSafeRandom equivalents too?


http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@152
PS3, Line 152:   T SelectAtRandom(const Container& c) {
Maybe rename these a bit to emphasize their similarity? Like, OneFromContainer 
and SomeFromContainer? Okay, those are pretty bad, but maybe something along 
those lines?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 03 Apr 2019 17:19:59 +
Gerrit-HasComments: Yes

[kudu-CR] util: pull Random methods out from tests

2019-04-03 Thread Andrew Wong (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: util: pull Random methods out from tests
..

util: pull Random methods out from tests

I've found a couple of Random utility methods pretty useful in some
tests I'm writing, so I'm pulling them out into util.

Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
---
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/random.h
2 files changed, 31 insertions(+), 28 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)