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/12 00:11:38 UTC
[kudu-CR] WIP: KUDU-738 (part 2) Always use pre-assigned timestamps in tablet tests
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5056
Change subject: WIP: KUDU-738 (part 2) Always use pre-assigned timestamps in tablet tests
......................................................................
WIP: KUDU-738 (part 2) Always use pre-assigned timestamps in tablet tests
This goes another step into removing safe time from mvcc. In this
patch we stop using ScopedTransactions in tablet tests with no
pre-assigned timestamps. This means that tests now need to share
an instance of LocalTabletWriter since timestamp assignement does
not retry until it finds an unassigned timestamp anymore.
Still left for a follow-up patch is the complete removal of safe time
from mvcc as that requires changing the tests that interct with
MvccManager directly.
Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
---
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_history_gc-test.cc
6 files changed, 167 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5056/1
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Patch Set 3:
(2 comments)
thanks for the input all. I've swapped this for a much less intrusive approach that only changes the tablet method and doesn't touch LocalTabletWriter.
http://gerrit.cloudera.org:8080/#/c/5056/3//COMMIT_MSG
Commit Message:
PS3, Line 7: -
> nit: may be, remove those extra symbols?
historically we've used these, but changed it to : and got rid of a space. (actually abandoned this patch but did it for the other ones).
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
Line 372: DCHECK(!tx_state->has_timestamp());
> might as well use a CHECK if this is only for tests
Done
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
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: Jean-Daniel Cryans <jd...@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] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has abandoned this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
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: Jean-Daniel Cryans <jd...@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] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/5056/3//COMMIT_MSG
Commit Message:
PS3, Line 7: -
nit: may be, remove those extra symbols?
PS3, Line 19: interct
typo
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:
PS3, Line 82: vector<Op> ops;
: ops.push_back(Op(type, &row));
nit: consider replacing with list-initializer style, i.e. remove the 'ops' variable at all and call WriteBatch like:
WriteBatch({ Op(type, &row) }, result);
PS3, Line 88: gscoped_ptr
nit: consider using std::unique_ptr instead
PS3, Line 104: NULL
nit: consider using nullptr instead of NULL.
PS3, Line 104: NULL
nit: consider using nullptr instead of NULL.
PS3, Line 151: TxResultPB result_;
Is it needed at all after the change?
PS3, Line 152: tserver::WriteRequestPB req_;
Is it needed at all after the change?
PS3, Line 153: gscoped_ptr<WriteTransactionState> tx_state_
Is this needed at all after the change?
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
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: Jean-Daniel Cryans <jd...@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] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:
PS3, Line 118: Since this a "local" tablet writer it shouldn't care about "safe"
: // time, which is a distributed concept.
> per my comment on the other commit, I'm a little nervous that there is some
note that this patch series, so far, does not change the behavior of followers, just the behavior of leaders. Leaders used to move safe time on commit but now all replicas behave the same way and safe time is only adjusted on the TransactionManager.
Interestingly we were advancing the safe time on the TransactionManager both for leaders and followers before the apply started which meant that even leader transactions would only advance the clean time and not the safe time on commit. So really this patch series doesn't introduce any change in behavior.
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
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: Jean-Daniel Cryans <jd...@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] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
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/5056
to look at the new patch set (#2).
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
This goes another step into removing safe time from mvcc. In this
patch we make tablet tests always use one instance of LocalTabletWriter
and use the same path as with regular transactions: i.e. always use
pre-assigned timestamps.
This means that tests now need to share an instance of LocalTabletWriter
since timestamp assignement does not retry until it finds an unassigned
timestamp anymore.
Still left for a follow-up patch is the complete removal of safe time
from mvcc as that requires changing the tests that interct with
MvccManager directly.
Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
---
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_history_gc-test.cc
6 files changed, 170 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5056/2
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
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/5056
to look at the new patch set (#3).
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
This goes another step into removing safe time from mvcc. In this
patch we make tablet tests always use one instance of LocalTabletWriter
and use the same path as with regular transactions: i.e. always use
pre-assigned timestamps.
This means that tests now need to share an instance of LocalTabletWriter
since timestamp assignement does not retry until it finds an unassigned
timestamp anymore.
Still left for a follow-up patch is the complete removal of safe time
from mvcc as that requires changing the tests that interct with
MvccManager directly.
Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
---
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_history_gc-test.cc
6 files changed, 171 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5056/3
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:
Line 96: CHECK_OK(SchemaToPB(*client_schema_, req.mutable_schema()));
> hrm, we have some benchmarks based on LocalTabletWriter IIRC -- this might
I didn't see anywhere that mt-tablet-test uses TxResultPB. In that case could keep result_ around and give last_op_result() thread-safe "last one wins" semantics.
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/tablet-test-base.h
File src/kudu/tablet/tablet-test-base.h:
Line 319: void InsertTestRows(LocalTabletWriter* writer,
Why not make the writer a member of this base class?
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
Line 372: DCHECK(!tx_state->has_timestamp());
might as well use a CHECK if this is only for tests
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
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: Jean-Daniel Cryans <jd...@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] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/local_tablet_writer.h
File src/kudu/tablet/local_tablet_writer.h:
Line 96: CHECK_OK(SchemaToPB(*client_schema_, req.mutable_schema()));
hrm, we have some benchmarks based on LocalTabletWriter IIRC -- this might be a negative perf change on the benchmarks. I guess the reason you did this is because now all of the threads share the same LocalTabletWriter instance? but really all they need to share is the lock_ simulating the single-threaded prepare, right? might be worth considering plumbing such a lock in instead of sharing the LocalTabletWriter instances, since LocalTabletWriter does a nice job of encapsulating the junk like TxResultPB which you've had to expose now
PS3, Line 118: Since this a "local" tablet writer it shouldn't care about "safe"
: // time, which is a distributed concept.
per my comment on the other commit, I'm a little nervous that there is some interaction between safe time and compaction snapshots, and we don't have a lot of tests which try to really stress compaction on a follower which might have transactions arriving during the various phases.
http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/tablet_history_gc-test.cc
File src/kudu/tablet/tablet_history_gc-test.cc:
Line 68: void InsertOriginalRows(LocalTabletWriter* writer, int num_rowsets, uint64_t rows_per_rowset);
can the writer just be a member variable set up in the ctor or Setup method? seems like they're all initialized the same way
--
To view, visit http://gerrit.cloudera.org:8080/5056
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@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