sql: re-enable txnidcache by default#77626
Conversation
|
This change is |
|
are there any workloads we know to be affected by this? |
|
Other than kv0/kv95, looking at the dashboard, I think all the YCSB workload were affected. |
|
I have been using YCSB-A to do stability testing for the last week (#75110), and I haven't observed any significant performance drops . I haven't tested other YCSB workloads yet. |
maryliag
left a comment
There was a problem hiding this comment.
Any other tests we should be doing before enabling this back?
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
Azhng
left a comment
There was a problem hiding this comment.
In terms of the tests we should run, two workload that have previously shown performance drops with TxnID Cache enabled are kv workload and ycsb workload.
- Since #77220, we have recovered all of the
kvperformance drop due to the expensive copies. - The nightlies also have been running with the contention event store turned on, and we haven't seen any significant performance impact there on roachperf.
- The YCSB-A workload that I have been running this week also have shown that, when both TxnID Cache and eventStore are enabled, there aren't any significant evidence of performance issue. (note: this is true even when running with an unstable binary with sub-optimal cluster settings, the YCSB-A throughput is still on-par with the baseline number, see #75110 for details.)
I think at this point, we could benefit a lot more from turning this on and keep this running in the nightly tests, and to learn more about its performance and stability.
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
Okay, I'm in favor of enabling it and keeping an eye. However, if there are noticeable regressions in the benchmarks, let's file an issue and flip it off rapidly.
Reviewable status:
Previously, TxnID cache was disabled by default due to performance issues. This has since been resolved in cockroachdb#77220. This commit re-enables TxnID Cache by default. Release justification: low risk high reward change. Release note: None
e5be17a to
b1f6722
Compare
maryliag
left a comment
There was a problem hiding this comment.
+1 to Andrew's comment. Let's keep an eye on regressions after this merge
Reviewable status:
|
TFTR! bors r=maryliag,ajwerner |
|
Build succeeded: |
Previously, TxnID cache was disabled by default due to performance
issues. This has since been resolved in #77220.
This commit re-enables TxnID Cache by default.
Release justification: low risk high reward change.
Release note: None