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 3) Remove automatic safe time adjustment from mvcc
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5057
Change subject: WIP KUDU-738 (part 3) Remove automatic safe time adjustment from mvcc
......................................................................
WIP KUDU-738 (part 3) Remove automatic safe time adjustment from mvcc
This completely removes that APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc.
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
WIP: Still some work to be done here.
---
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
2 files changed, 3 insertions(+), 134 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/1
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#11).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However, this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 194 insertions(+), 283 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/11
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#10).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However, this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 185 insertions(+), 274 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/10
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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 3) - Remove automatic safe time adjustment from mvcc
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 5:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
PS5, Line 217: TestOfflineTransactions
can you update the terminology and the comment here? we don't use the word 'offline' in MVCC anymore so this test is now confusing
Line 228: // and committing this transaction "offline" this
same
Line 546: // coalesce to the latest timestamp, for offline transactions.
same (and elsewhere -- grep for 'offline' in this test)
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
PS5, Line 54: There is already a transaction with timestamp: $0 in flight
this error string isn't necessarily accurate in the case that InitTransactionUnlocked returns false due to timestamp being lower than no_new_transactions_at_or_before_.
I also noticed this comment in InitTransactionUnlocked:
// Ensure that we didn't mark the given timestamp as "safe" in between
// acquiring the time and taking the lock. This allows us to acquire timestamps
// outside of the MVCC lock.
This comment was added by b6f141f37c917ff1bc2607dc6808a680ca2b2c7d which has some interesting notes in its commit message about lock contention in the MVCC stuff. Might be worth doing a before/after sanity check of perf using the benchmark invocations mentioned in that message. Additionally, I think the comment above probably needs to be removed or updated, since I don't entirely understand it anymore.
PS5, Line 55:
weird indent
PS5, Line 125: // If this transaction was the earliest in-flight, we might have to adjust
: // the "clean" timestamp.
: AdjustCleanTime();
ah, so my comment in the other review about clean/safe time not having a 'subset' relationship is actually false. Here you are still only updating the 'clean' time up to the 'safe' time if I understand correctly, and never above.
PS5, Line 182: no_new_transactions_at_or_before_
should we rename this member variable to 'safe_time_'?
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS5, Line 168: // When a transaction is started, a timestamp is assigned. The manager will
: // never assign a timestamp if there is already another transaction with
: // the same timestamp in flight or previously committed.
needs to be updated
PS5, Line 203: without advancing the safe time
this probably needs to be updated, right?
PS5, Line 211: // Used for bootstrap and delayed processing in FOLLOWERS/LEARNERS.
this is used in all cases now
Line 218: void AdjustSafeTime(Timestamp safe_time);
can you add a note about when this is expected to be called on leaders vs followers etc?
Line 381: explicit ScopedTransaction(MvccManager* manager, Timestamp timestamp);
can remove 'explicit' here
Line 411: Timestamp timestamp_;
can be const now
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 5
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
[kudu-CR] KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
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/5057
to look at the new patch set (#4).
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
This completely removes that APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc. Previous patches
had taken care of making sure these were unused in regular
TabletServer operations or in tests that use LocalTabletWriter.
This patch takes this the rest of the way by removing the APIs
from Mvcc completely and changing tests that use it directly
to obtain the timestamps from a clock.
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 134 insertions(+), 214 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/4
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 4
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) Remove automatic safe time adjustment from mvcc
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) Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 5:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/5057/5//COMMIT_MSG
Commit Message:
PS5, Line 9: This
> nit: This --> This patch
Done
PS5, Line 9: that
> nit: that --> the or that --> those or, may be, drop 'that'?
Done
PS5, Line 14: This patch takes this the rest of the way by
> nit: may be 'This patch completes that by ...'
Done
PS5, Line 10: Previous patches
: had taken care of making sure these were unused in regular
: TabletServer operations or in tests that use LocalTabletWriter.
:
: This patch takes this the rest of the way by removing the APIs
: from Mvcc completely and changing tests that use it directly
: to obtain the timestamps from a clock.
> nit: may be, separate this into its own paragraph?
simplified, I don't think it needs it now
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
PS5, Line 174: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t
> Here and elsewhere: consider removing the temporary variable
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/deltamemstore-test.cc
File src/kudu/tablet/deltamemstore-test.cc:
PS5, Line 82: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
> nit: consider instead
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/diskrowset-test.cc
File src/kudu/tablet/diskrowset-test.cc:
PS5, Line 335: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
> nit: consider
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/memrowset-test.cc
File src/kudu/tablet/memrowset-test.cc:
PS5, Line 110: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
> Here and elsewhere, consider
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
PS5, Line 101: TEST_F(MvccTest, TestMvccMultipleInFlight)
> It would be nice to have a concise description for the idea of the test: wh
this test is not new nor is it changed in any way funcionally. would prefer to avoid fixing these random backlog things in what are mostly mechanical changes.
PS5, Line 105: // Start timestamp 1, timestamp 2
> What does 'Start timestamp' mean? May be, update this comment to clarify t
removed
PS5, Line 164: mgr.AdjustSafeTime(t3);
> Why did it appear in this revision of the test? Does this mean the previou
this patch is about removing automatic safe time adjustment apis from mvcc. since we don't have the automatic apis anymore we have to do it "manually" (this would be called within CommitTransaction above). Added a comment to that regard though honestly don't know whether it conveyed much more information than the code statement.
PS5, Line 217: TestOfflineTransactions
> can you update the terminology and the comment here? we don't use the word
Done
Line 228: // and committing this transaction "offline" this
> same
Done
Line 546: // coalesce to the latest timestamp, for offline transactions.
> same (and elsewhere -- grep for 'offline' in this test)
Done
PS5, Line 552: CHECK_OK
> Why not CHECK_OK(), not ASSERT_OK() ?
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
PS5, Line 54: There is already a transaction with timestamp: $0 in flight
> this error string isn't necessarily accurate in the case that InitTransacti
yeah this race can't happen anymore because ts assignment and txn start are done under a lock now, for test. did a sanity check. there was a slowdown of about 10% on mt-tablet-test, but this is not the actual tserver write patch. ran full_stack-insert-scan-test there are there is actually a speedup of 9%.
PS5, Line 55:
> weird indent
Done
PS5, Line 125: // If this transaction was the earliest in-flight, we might have to adjust
: // the "clean" timestamp.
: AdjustCleanTime();
> ah, so my comment in the other review about clean/safe time not having a 's
yeah, safe/clean time behave as before. As I mentioned on the other patch (funnily or not :)) until now this patch series doesn't actually change any behavior. We were advancing "safe" time before apply. meaning that the "automatic' advancement on commit for leader txns would only really either move the "clean" time or nothing at all, which is what still happens now.
PS5, Line 182: no_new_transactions_at_or_before_
> should we rename this member variable to 'safe_time_'?
Done
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS5, Line 168: // When a transaction is started, a timestamp is assigned. The manager will
: // never assign a timestamp if there is already another transaction with
: // the same timestamp in flight or previously committed.
> needs to be updated
Done
PS5, Line 203: without advancing the safe time
> this probably needs to be updated, right?
Done
PS5, Line 211: // Used for bootstrap and delayed processing in FOLLOWERS/LEARNERS.
> this is used in all cases now
Done
Line 218: void AdjustSafeTime(Timestamp safe_time);
> can you add a note about when this is expected to be called on leaders vs f
Part of the point of this patch series is to make mvcc oblivious to leaders/followers so would rather not mention them specifically, just the general constraints around 'safe_time'.
Pointed to the WIP doc (that is not merged) on the class header as it will be the place where these concepts live. Afraid I will forget if I postpone linking for later and doesn't seem jira worthy.
Line 381: explicit ScopedTransaction(MvccManager* manager, Timestamp timestamp);
> can remove 'explicit' here
Done
Line 411: Timestamp timestamp_;
> can be const now
Done
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 5
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) Remove automatic safe time adjustment from mvcc
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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: No
[kudu-CR] KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#8).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because for non-tserver based tests timestamp assignment and
transaction start are now done under a lock, this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (perf stats in the end
of this commit message).
However this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance we were advancing safe time twice for
leader side txns, one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 184 insertions(+), 273 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/8
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#6).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc.
The previous patch had taken care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because timestamp assignment and transaction start are now done
under a lock, this does have some impact on some tests like
mt-tablet-test. I measured about 10% slowdown on an mvcc-stressing
config of mt-tablet-test (perf stats in the end of this commit
message).
However this patch series does not add any overhead on the actual
tablet server write path. In fact this even simplifies it in some
circumstances (for instance we were advancing safe time twice for
leader side txns, one when it was consensus committed and one when
the apply was complete). Due to this there was even a moderate
perf gain in this more important case of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 184 insertions(+), 273 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/6
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 6
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>
[kudu-CR] KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#9).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 184 insertions(+), 273 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/9
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 9
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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#12).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However, this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 197 insertions(+), 295 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/12
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 12
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) Remove automatic safe time adjustment from mvcc
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
PS10, Line 228: / and committing this transaction this should not advance the MvccManager
: // 'all_committed_before_' watermark.
not quite a sentence
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
PS10, Line 48: // This shouldn't happen in general, so in debug more just crash.
hrm, maybe this shoudl just be a CHECK then? if this happens can't we get all sorts of bizarre corruption issues that would be hard to track down, and crashing would be safer for data?
Line 58: strings::Substitute("There is already a transaction with timestamp: $0 in flight or "
same, do we ever expect this to happen?
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS10, Line 186: 'all_committed_before_
should this be referring to safe_time_ now? seems this all_committed_before_ isn't an actual field name
PS10, Line 220: with timestamp
'with timestamps equal to'
PS10, Line 220: anymore
nit: 'any more'
PS10, Line 360: ID
timestamp
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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 3) Remove automatic safe time adjustment from mvcc
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/5057
to look at the new patch set (#2).
Change subject: KUDU-798 (part 3) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 3) Remove automatic safe time adjustment from mvcc
This completely removes that APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc. Previous patches
had taken care of making sure these were unused in regular
TabletServer operations or in tests that use LocalTabletWriter.
This patch takes this the rest of the way by removing the APIs
from Mvcc completely and changing tests that use it directly
to obtain the timestamps from a clock.
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 133 insertions(+), 214 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/2
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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 3) - Remove automatic safe time adjustment from mvcc
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 5:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/5057/5//COMMIT_MSG
Commit Message:
PS5, Line 9: that
nit: that --> the or that --> those or, may be, drop 'that'?
PS5, Line 9: This
nit: This --> This patch
PS5, Line 14: This patch takes this the rest of the way by
nit: may be 'This patch completes that by ...'
PS5, Line 10: Previous patches
: had taken care of making sure these were unused in regular
: TabletServer operations or in tests that use LocalTabletWriter.
:
: This patch takes this the rest of the way by removing the APIs
: from Mvcc completely and changing tests that use it directly
: to obtain the timestamps from a clock.
nit: may be, separate this into its own paragraph?
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
PS5, Line 174: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t
Here and elsewhere: consider removing the temporary variable
ScopedTransaction tx(&mvcc_, clock_->Now());
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/deltamemstore-test.cc
File src/kudu/tablet/deltamemstore-test.cc:
PS5, Line 82: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
nit: consider instead
ScopedTransaction tx(&mvcc_, clock_->Now());
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/diskrowset-test.cc
File src/kudu/tablet/diskrowset-test.cc:
PS5, Line 335: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
nit: consider
ScopedTransaction tx(&mvcc_, clock_->Now());
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/memrowset-test.cc
File src/kudu/tablet/memrowset-test.cc:
PS5, Line 110: Timestamp t = clock_->Now();
: ScopedTransaction tx(&mvcc_, t);
Here and elsewhere, consider
ScopedTransaction tx(&mvcc_, clock_->Now());
http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
PS5, Line 101: TEST_F(MvccTest, TestMvccMultipleInFlight)
It would be nice to have a concise description for the idea of the test: what exactly it verifies and how.
PS5, Line 105: // Start timestamp 1, timestamp 2
What does 'Start timestamp' mean? May be, update this comment to clarify that or remove the comment at all -- if it's just starting transaction at timestamp, it's clear from the code itself.
PS5, Line 164: mgr.AdjustSafeTime(t3);
Why did it appear in this revision of the test? Does this mean the previous revisions did something different?
Overall, it would be nice to have a comment explaining why the safe time has been adjusted.
PS5, Line 552: CHECK_OK
Why not CHECK_OK(), not ASSERT_OK() ?
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 5
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
[kudu-CR] KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
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) Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
PS10, Line 228: / and committing this transaction this should not advance the MvccManager
: // 'all_committed_before_' watermark.
> not quite a sentence
Done
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
PS10, Line 48: // This shouldn't happen in general, so in debug more just crash.
> hrm, maybe this shoudl just be a CHECK then? if this happens can't we get a
when this returns IllegalState it causes a crash on ScopedTransaction, but I think you're right that is makes more sense to crash here. Done
Line 58: strings::Substitute("There is already a transaction with timestamp: $0 in flight or "
> same, do we ever expect this to happen?
same
http://gerrit.cloudera.org:8080/#/c/5057/10/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS10, Line 186: 'all_committed_before_
> should this be referring to safe_time_ now? seems this all_committed_before
removed this since this method no longer returns a status.
PS10, Line 220: with timestamp
> 'with timestamps equal to'
Done
PS10, Line 220: anymore
> nit: 'any more'
Done
PS10, Line 360: ID
> timestamp
Done
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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 3) - Remove automatic safe time adjustment from mvcc
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/5057
to look at the new patch set (#3).
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
This completely removes that APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc. Previous patches
had taken care of making sure these were unused in regular
TabletServer operations or in tests that use LocalTabletWriter.
This patch takes this the rest of the way by removing the APIs
from Mvcc completely and changing tests that use it directly
to obtain the timestamps from a clock.
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 133 insertions(+), 214 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/3
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#7).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because for non-tserver based tests timestamp assignment and
transaction start are now done under a lock, this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (perf stats in the end
of this commit message).
However this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance we were advancing safe time twice for
leader side txns, one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 184 insertions(+), 273 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/7
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 7
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 3) - Remove automatic safe time adjustment from mvcc
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 5
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: No
[kudu-CR] KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
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/5057
to look at the new patch set (#5).
Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
This completely removes that APIs that allow automatic timestamp
assignment and safe time adjustment from Mvcc. Previous patches
had taken care of making sure these were unused in regular
TabletServer operations or in tests that use LocalTabletWriter.
This patch takes this the rest of the way by removing the APIs
from Mvcc completely and changing tests that use it directly
to obtain the timestamps from a clock.
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 137 insertions(+), 217 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/5
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 5
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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5057
to look at the new patch set (#13).
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However, this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 196 insertions(+), 294 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/13
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
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: 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) Remove automatic safe time adjustment from mvcc
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.
Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
......................................................................
KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc
This patch completely removes the APIs that allow automatic timestamp
assignment and automatic safe time adjustment from Mvcc.
The previous patch took care of making sure these APIs were
unused in regular TabletServer operations. This patch completes
that by removing the APIs from Mvcc and changing tests appropriately.
Because, for non-tserver based tests, timestamp assignment and
transaction start are now done under a lock this does introduce
a slowdown in tests like mt-tablet-test. I measured about 10% slowdown
on an mvcc-stressing config of mt-tablet-test (run config and
perf stats in the end of this commit message).
However, this patch series does not add any overhead on the actual
tablet server write path. In fact, this even simplifies it in some
circumstances: for instance, we were advancing safe time twice for
leader side txns: one when it was consensus committed and one when
the apply was complete. Due to this, there was even a moderate
speedup in this more important case, measured with
full_stack-insert-scan-test, (run config and perf stats below)
of about 9%.
===================================================================
full_stack-insert-scan-test run config:
bin/full_stack-insert-scan-test \
--gtest_filter=*MRSOnlyStressTest* \
--inserts_per_client=200000 \
--concurrent_inserts=10 \
--rows_per_batch=1 \
--skip_scans
Results on master before this patch series:
344086.766163 task-clock # 5.659 CPUs utilized
14,435,389 context-switches # 0.042 M/sec
91,225 cpu-migrations # 0.265 K/sec
112,036 page-faults # 0.326 K/sec
956,637,266,040 cycles # 2.780 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
577,637,169,921 instructions # 0.60 insns per cycle
109,720,205,884 branches # 318.874 M/sec
730,807,372 branch-misses # 0.67% of all branches
60.807013290 seconds time elapsed
Results on master after this patch series:
328574.272742 task-clock # 5.932 CPUs utilized
13,820,330 context-switches # 0.042 M/sec
170,370 cpu-migrations # 0.519 K/sec
111,997 page-faults # 0.341 K/sec
911,264,247,114 cycles # 2.773 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
572,898,176,369 instructions # 0.63 insns per cycle
108,872,113,035 branches # 331.347 M/sec
728,934,228 branch-misses # 0.67% of all branches
55.387672052 seconds time elapsed
===================================================================
mt-tablet-test run config:
bin/mt-tablet-test \
--gtest_filter=*0*DoTestAllAtOnce* \
--num_counter_threads=0 \
--num_slowreader_threads=0 \
--flusher_backoff=1.0 \
--flusher_initial_frequency_ms=10000 \
--inserts_per_thread=1000000 \
--num_summer_threads=0 \
--gtest_repeat=3 \
--tablet_test_flush_threshold_mb=2000 \
--minloglevel=10
Results on master before this patch series:
618894.806683 task-clock # 7.716 CPUs utilized
7,243,713 context-switches # 0.012 M/sec
782 cpu-migrations # 0.001 K/sec
1,457,677 page-faults # 0.002 M/sec
1,794,712,092,284 cycles # 2.900 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
648,757,335,519 instructions # 0.36 insns per cycle
128,766,989,780 branches # 208.060 M/sec
790,345,583 branch-misses # 0.61% of all branches
80.204388663 seconds time elapsed
Result on master after this patch series:
620594.911121 task-clock # 7.012 CPUs utilized
17,645,922 context-switches # 0.028 M/sec
1,163 cpu-migrations # 0.002 K/sec
1,252,812 page-faults # 0.002 M/sec
1,775,937,664,119 cycles # 2.862 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
757,882,564,990 instructions # 0.43 insns per cycle
155,500,757,477 branches # 250.567 M/sec
982,110,478 branch-misses # 0.63% of all branches
88.500973260 seconds time elapsed
Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Reviewed-on: http://gerrit.cloudera.org:8080/5057
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
13 files changed, 196 insertions(+), 294 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 14
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>