[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-10 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-10 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 13:

Build Started http://104.196.14.100/job/kudu-gerrit//

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-10 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 12:

Build Started http://104.196.14.100/job/kudu-gerrit/3332/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-10 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 10:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3975/10/src/kudu/integration-tests/tablet_history_gc-itest.cc
File src/kudu/integration-tests/tablet_history_gc-itest.cc:

Line 125: ACTIONS_NUM_ITEMS = 8, // Count of items in this enum. Update if 
needed.
> why assign the integers here explicitly? the enum is already defined to go 
Done


Line 127:   static const int kNumActions = ACTIONS_NUM_ITEMS;
> why not just use kNumActions as the enum value on line 125?
Done


Line 130:   using MaterializedTestTable = std::map;
> oh shit! c++11 'using' declarations! I award thee 3 Burkert Points for usag
:D


PS10, Line 156: auto iter = snapshots_.upper_bound(ts.ToUint64());
  : if (iter == snapshots_.begin()) {
  :   LOG(FATAL) << "There is no saved snapshot with a TS <= " 
<< ts.ToUint64();
  : }
  : --iter;
  : VLOG(3) << "Using verification snapshot from TS " << 
StringifyTimestamp(Timestamp(iter->first));
  : return >second;
  :  
> can you use FindFloorOrDie here? (or FindFloorOrNull if you want to keep th
Thanks, that's exactly this logic.


Line 186: scanners_.insert(std::move(entry));
> could you do this a little more succintly with:
I tried that when writing this. Just tried a bunch of things again and I 
couldn't get them to work... Having a tough time figuring out exactly why... I 
just get template mismatch errors.


PS10, Line 192:   
NO_FATALS(VerifySnapshotScan(std::move(iter->second.first),
  : std::move(iter->second.second), iter->first));
  :  
> all these firsts/seconds are a bit confusing (especially because the typede
Done


Line 196:   scanners_.erase(old_iter);
> somewhat surprising that this function actually removes the iterator elemen
Did the 2nd thing


PS10, Line 214:   if (snap == nullptr) {
  :   FAIL() << "Could not find snapshot to match timestamp " 
<< snap_ts.ToString();
  : }
> how about just ASSERT_NOTNULL(snap) << ...
used ASSERT_NE since ASSERT_NOTNULL has a return value


PS10, Line 423:  (random.OneIn(20) == 0);
> I think this condition is wrong -- OneIn(20) is already a boolean true 5% o
Thanks for the catch


PS10, Line 482: (random.OneIn(20) == 0);
> same
Done


Line 560: //int read_delay_rounds = 
random.Uniform(FLAGS_test_num_rounds / 10);
> remove
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-09 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 10:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3975/10/src/kudu/integration-tests/tablet_history_gc-itest.cc
File src/kudu/integration-tests/tablet_history_gc-itest.cc:

Line 125: ACTIONS_NUM_ITEMS = 8, // Count of items in this enum. Update if 
needed.
why assign the integers here explicitly? the enum is already defined to go from 
0 to the number of elements


Line 127:   static const int kNumActions = ACTIONS_NUM_ITEMS;
why not just use kNumActions as the enum value on line 125?


Line 130:   using MaterializedTestTable = std::map;
oh shit! c++11 'using' declarations! I award thee 3 Burkert Points for usage of 
new features.


PS10, Line 156: auto iter = snapshots_.upper_bound(ts.ToUint64());
  : if (iter == snapshots_.begin()) {
  :   LOG(FATAL) << "There is no saved snapshot with a TS <= " 
<< ts.ToUint64();
  : }
  : --iter;
  : VLOG(3) << "Using verification snapshot from TS " << 
StringifyTimestamp(Timestamp(iter->first));
  : return >second;
  :  
can you use FindFloorOrDie here? (or FindFloorOrNull if you want to keep the 
nicer fatal message)


Line 186: scanners_.insert(std::move(entry));
could you do this a little more succintly with:

scanners_.emplace(verify_round, {std::move(scanner), snap_ts});


PS10, Line 192:   
NO_FATALS(VerifySnapshotScan(std::move(iter->second.first),
  : std::move(iter->second.second), iter->first));
  :  
all these firsts/seconds are a bit confusing (especially because the typedefs 
mean the type parameters aren't visible anywhere near here). Mind unpacking 
them into local variables with names first?


Line 196:   scanners_.erase(old_iter);
somewhat surprising that this function actually removes the iterator elements. 
Maybe make this pure iteration, and then just use scanners_.erase(...) at the 
call site to erase the range? (or clear in the case of AllRemaining)

alternatively, rename this to VerifyAndRemoveScanners or something?


PS10, Line 214:   if (snap == nullptr) {
  :   FAIL() << "Could not find snapshot to match timestamp " 
<< snap_ts.ToString();
  : }
how about just ASSERT_NOTNULL(snap) << ...


PS10, Line 423:  (random.OneIn(20) == 0);
I think this condition is wrong -- OneIn(20) is already a boolean true 5% of 
the time


PS10, Line 482: (random.OneIn(20) == 0);
same


Line 560: //int read_delay_rounds = 
random.Uniform(FLAGS_test_num_rounds / 10);
remove


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-09 Thread Mike Percy (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..

KUDU-236 (part 2). Create randomized tablet history GC itest

Randomized test that performs random operations on a tablet and then
verifies that data is not lost as tablet history GC randomly occurs.

It may be useful to add the following features to this test in follow-up
commits:

* Support for reinsert (requires KUDU-237)
* Control over rowsets selected for compaction
* Control over projections selected for compaction

Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
---
A src/kudu/integration-tests/mini_cluster-itest-base.h
M src/kudu/integration-tests/tablet_history_gc-itest.cc
2 files changed, 604 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-09 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 11:

Build Started http://104.196.14.100/job/kudu-gerrit/3320/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-09 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/3313/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-09-02 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/3216/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-08-15 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 7:

I ran 3000 loops of this with 3000 rounds each and they all passed (in RELEASE 
mode). See http://dist-test.cloudera.org/job?job_id=mpercy.1471276568.15474

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-08-15 Thread Mike Percy (Code Review) 
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..

KUDU-236 (part 2). Create randomized tablet history GC itest

Randomized test that performs random operations on a tablet and then
verifies that data is not lost as tablet history GC randomly occurs.

It may be useful to add the following features to this test in follow-up
commits:

* Support for reinsert (requires KUDU-237)
* Control over rowsets selected for compaction
* Control over projections selected for compaction

Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
---
A src/kudu/integration-tests/mini_cluster-itest-base.h
M src/kudu/integration-tests/tablet_history_gc-itest.cc
2 files changed, 605 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-08-15 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..


Patch Set 7:

Build Started http://104.196.14.100/job/kudu-gerrit/2904/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-236 (part 2). Create randomized tablet history GC itest

2016-08-15 Thread Mike Percy (Code Review) 
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest
..

KUDU-236 (part 2). Create randomized tablet history GC itest

Randomized test that performs random operations on a tablet and then
verifies that data is not lost as tablet history GC randomly occurs.

It may be useful to add the following features to this test in follow-up
commits:

* Support for reinsert (requires KUDU-237)
* Control over rowsets selected for compaction
* Control over projections selected for compaction

Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
---
A src/kudu/integration-tests/mini_cluster-itest-base.h
M src/kudu/integration-tests/tablet_history_gc-itest.cc
2 files changed, 605 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon