You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/11/08 08:49:29 UTC

[kudu-CR] Add Reinserts to tablet history gc-itest 

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................

Add Reinserts to tablet_history_gc-itest

This adds Reinserts as a new operation to tablet_history_gc-itest.
Reinserts happen on previously deleted rows and, like deletes,
have a change to happen mid-compaction.

This test used to fail frequently before the reinsert patches
are in and now passes.

WIPish Will likely get a few dist-test runs for this, though
this was already flaky so we'll see how troublesome that is..

Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 90 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 10:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/4997/10//COMMIT_MSG
Commit Message:

Line 17: this was already flaky so we'll see how troublesome that is..
> I'm looking into this right now, trying to pick up from where Alexey left o
I unflaked the test prior to this patch by reverting to manual flush


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

Line 21: 
> nit: please add <unordered_set> include file
Done


PS10, Line 286: int32_t rows_inserted_ = 0;
> Consider removing this and replacing with a local variable, like rows_delet
Done


PS10, Line 317: SetStringCopy
> nit: SetString() now does copying; so consider replacing SetStringCopy() wi
Done


Line 396:         // TODO: Allow for reinsert onto deleted rows after KUDU-237 has been
> Remove this comment, also should we consider treating insert and reinsert a
the mechanics are quite different and it would be a bit of a refactoring so I'm going to push back on this. While cleaner in a sense I don't think it would test more than we're testing now.


PS10, Line 506: MaterializedTestRow test_row
> Why not to use reference instead?  Why is it necessary to make a copy and t
we don't take non-const references as per our style. changed this to a pointer


PS10, Line 507: CHECK_EQ
> Why not ASSERT_EQ() ?
Done


PS10, Line 556: MaterializedTestTable snapshot = CloneLatestSnapshot();
> Is it crucial to make a snapshot every time regardless of number of deleted
Done


PS10, Line 565: break
> Consider moving this up into the very beginning of the scope next to the 'i
Done


PS10, Line 567: reinserts
> nit: consider adding call to reserve the space:
regardless of the number of num_rows_to_reinsert we only do so for rows that were previously deleted so we're not really sure how big the vector needs to be. You do make a good point about the name of the var. Will change it to max_rows_to_reinsert.


PS10, Line 569: int32_t
> nit: consider adding const specifier for better readability:
Done


Line 569:           int32_t row_key = *std::next(deleted_rows_.begin(), random.Uniform(num_deleted_rows));
> hrm, O(k*n) kind of sucks, i wonder if this needs to be made faster
maybe it could, but this is not perf-critical code, right? so, does it matter? I doubt very much that this shows up on any perf trace we take of this test.


PS10, Line 570: MaterializedTestRow test_row
> Why not to use a reference instead of copy here?  If using reference, then 
we don't take non-const references as per our style. changed this to a pointer


PS10, Line 571: CHECK_EQ
> Why not just ASSERT_EQ() ?
Done


PS10, Line 602: 20000
> nit: may be, create a constant for this timeout and use it here and in all 
Done


PS10, Line 617: VLOG(1) << "Reinserted " << reinserts.size() << " rows";
> Is the 'reinserts' container valid at this point?  There is set_reinserts(s
ah good catch. fixed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................

Add Reinserts to tablet_history_gc-itest

This adds Reinserts as a new operation to tablet_history_gc-itest.
Reinserts happen on previously deleted rows and, like deletes,
have a change to happen mid-compaction.

This test used to fail frequently before the reinsert patches
are in and now passes.

Ran 500 loops of tablet_history_gc-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4

All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480101254.2208

Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 121 insertions(+), 37 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................

Add Reinserts to tablet_history_gc-itest

This adds Reinserts as a new operation to tablet_history_gc-itest.
Reinserts happen on previously deleted rows and, like deletes,
have a change to happen mid-compaction.

This test used to fail frequently before the reinsert patches
are in and now passes.

WIPish Will likely get a few dist-test runs for this, though
this was already flaky so we'll see how troublesome that is..

Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 90 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4997/10//COMMIT_MSG
Commit Message:

Line 17: this was already flaky so we'll see how troublesome that is..
yea, I'd forgotten about this flakiness in this test. I'll take a look at that thread which we were discussing in october and see if I can understand the base flakiness.

IIRC the flakiness started when we switched to AUTO_FLUSH_BACKGROUND in this test, so if you want a stable base, maybe we can flip back to manual flush?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Add Reinserts to tablet_history_gc-itest

This adds Reinserts as a new operation to tablet_history_gc-itest.
Reinserts happen on previously deleted rows and, like deletes,
have a change to happen mid-compaction.

This test used to fail frequently before the reinsert patches
are in and now passes.

Ran 500 loops of tablet_history_gc-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4

All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480101254.2208

Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Reviewed-on: http://gerrit.cloudera.org:8080/4997
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 121 insertions(+), 37 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 8:

(1 comment)

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

PS8, Line 618: //break
This looks a little bit suspicious.  Is this a typo or it's an intentional fall-through?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................

Add Reinserts to tablet_history_gc-itest

This adds Reinserts as a new operation to tablet_history_gc-itest.
Reinserts happen on previously deleted rows and, like deletes,
have a change to happen mid-compaction.

This test used to fail frequently before the reinsert patches
are in and now passes.

WIPish Will likely get a few dist-test runs for this, though
this was already flaky so we'll see how troublesome that is..

Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
---
M src/kudu/integration-tests/tablet_history_gc-itest.cc
1 file changed, 90 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4997/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4997
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 11: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 8:

(1 comment)

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

PS8, Line 618: //break
> This looks a little bit suspicious.  Is this a typo or it's an intentional 
ah, good catch. intentional for tests (to force flushes right after reinserts to stress that path more). will remove.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 10:

(2 comments)

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

Line 396:         // TODO: Allow for reinsert onto deleted rows after KUDU-237 has been
Remove this comment, also should we consider treating insert and reinsert as basically the same case for this test? It's probably better to have the possibility of batches with a combination of inserts and reinserts anyway.


Line 569:           int32_t row_key = *std::next(deleted_rows_.begin(), random.Uniform(num_deleted_rows));
hrm, O(k*n) kind of sucks, i wonder if this needs to be made faster


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4997/10//COMMIT_MSG
Commit Message:

Line 17: this was already flaky so we'll see how troublesome that is..
> yea, I'd forgotten about this flakiness in this test. I'll take a look at t
I'm looking into this right now, trying to pick up from where Alexey left off last month.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
......................................................................


Patch Set 10:

(13 comments)

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

Line 21: 
nit: please add <unordered_set> include file


PS10, Line 286: int32_t rows_inserted_ = 0;
Consider removing this and replacing with a local variable, like rows_deleted, rows_updated, rows_reinserted.


PS10, Line 317: SetStringCopy
nit: SetString() now does copying; so consider replacing SetStringCopy() with the shorter alias


PS10, Line 506: MaterializedTestRow test_row
Why not to use reference instead?  Why is it necessary to make a copy and then re-place the element at the same key with the copy later with

snapshot[row_key] = std::move(test_row) ?


PS10, Line 507: CHECK_EQ
Why not ASSERT_EQ() ?


PS10, Line 556: MaterializedTestTable snapshot = CloneLatestSnapshot();
Is it crucial to make a snapshot every time regardless of number of deleted rows so far?  If not, consider moving this after the 'if (num_deleted_rows == 0)' check


PS10, Line 565: break
Consider moving this up into the very beginning of the scope next to the 'if (rows_inserted_ == 0)' check.

nit: consider using 'continue' here as well as in earlier checks.  Or vice versa.


PS10, Line 567: reinserts
nit: consider adding call to reserve the space:

reinserts.reserve(num_rows_to_reinsert);


PS10, Line 569: int32_t
nit: consider adding const specifier for better readability:

const int32_t row_key = ...;


PS10, Line 570: MaterializedTestRow test_row
Why not to use a reference instead of copy here?  If using reference, then no need to replace (copy-over) it later calling something like 'snapshot[row_key] = std::move(test_row)'


PS10, Line 571: CHECK_EQ
Why not just ASSERT_EQ() ?


PS10, Line 602: 20000
nit: may be, create a constant for this timeout and use it here and in all other places in this test?


PS10, Line 617: VLOG(1) << "Reinserted " << reinserts.size() << " rows";
Is the 'reinserts' container valid at this point?  There is set_reinserts(std::move(reinserts)) call prior to that in case of force_reupdate_missed_deltas


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes