Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/21447 )
Change subject: Add a benchmark for CBTree concurrent writes. ...................................................................... Patch Set 4: (17 comments) http://gerrit.cloudera.org:8080/#/c/21447/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21447/1//COMMIT_MSG@32 PS1, Line 32: > nit: writer ? ok http://gerrit.cloudera.org:8080/#/c/21447/1//COMMIT_MSG@37 PS1, Line 37: erfor > nit: each ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@31 PS1, Line 31: #include <gtest/gtest.h> > It seems you'll need to rebase this patch on top the HEAD revision in the m ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@907 PS1, Line 907: > nit: consider ether converting this into a functor visible only in the scop Thanks. I will use std::array and make it a lambda http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@924 PS1, Line 924: TEST_F(TestCBTree, TestConcurrentReadWritePerformance) { > style nit here and below: use kCamelName pattern for static/constexpr const ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@925 PS1, Line 925: SKIP_IF_SLOW_NOT_ALLOWED(); : constexpr int kTrials = 10; > Do you want these to be hard-coded, or there might be some extra value in d I added 4 flags. Number of r/w threads, number of inserts (no longer required to be a prime or p % 4 == 3, It felt really user unfriendly), and a num_reader_boost to increase the workload of reader threads (they are much faster than writer thread). Actually num_reader_boost might be the most useful one. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@928 PS1, Line 928: her > ? *easy http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@931 PS1, Line 931: > nit: does LINT and 'git check' approve using this symbol? Yes, they seem to be cool with it. I change it to p % 4 == 3. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@938 PS1, Line 938: tatic_c > nit: barriers? ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@958 PS1, Line 958: nsert ra > style nit for here and below: is it possible to use 'while (true)' to match Yes. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1024 PS1, Line 1024: > nit: threads ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1028 PS1, Line 1028: tree.reset(); > Does it make sense to check for failures before starting the second phase o yes, it does. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1029 PS1, Line 1029: > nit: threads ok http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1029 PS1, Line 1029: > nit: $2 values ? ok http://gerrit.cloudera.org:8080/#/c/21447/3/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21447/3/src/kudu/tablet/cbtree-test.cc@494 PS3, Line 494: break; This is just because the rebase. Nothing changes here http://gerrit.cloudera.org:8080/#/c/21447/3/src/kudu/tablet/cbtree-test.cc@501 PS3, Line 501: for (thread& thr : threads) { This is just because the rebase. Nothing changes here http://gerrit.cloudera.org:8080/#/c/21447/3/src/kudu/tablet/cbtree-test.cc@759 PS3, Line 759: break; This is just because the rebase. Nothing changes here -- To view, visit http://gerrit.cloudera.org:8080/21447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b0b16e269c70716962fc5ebb4ddca1e2cbe68a4 Gerrit-Change-Number: 21447 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Thu, 23 May 2024 22:28:00 +0000 Gerrit-HasComments: Yes
