Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21447 )
Change subject: Add a benchmark for CBTree concurrent writes. ...................................................................... Patch Set 1: Code-Review+1 (15 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: writing nit: writer ? Otherwise, maybe re-phrase it to be "Reading threads ... writing threads ..." http://gerrit.cloudera.org:8080/#/c/21447/1//COMMIT_MSG@37 PS1, Line 37: eachi nit: each 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 "kudu/gutil/stringprintf.h" It seems you'll need to rebase this patch on top the HEAD revision in the master branch of the repo to get '#include "kudu/gutil/atomicops.h"' that IWYU wants to see here. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@907 PS1, Line 907: snprintf(kbuf, 64, "key_%d", i); nit: consider ether converting this into a functor visible only in the scope where the first two arguments are defined (and maybe even using sizeof(...) instead of hard-coded constant, if possible) or change the signature to supply information on the size of the buffers. Alternatively, use std::array for the first two parameters. http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@924 PS1, Line 924: constexpr int trials = 10; style nit here and below: use kCamelName pattern for static/constexpr constants http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@925 PS1, Line 925: constexpr int num_writer_threads = 9; : constexpr int num_reader_threads = 10; Do you want these to be hard-coded, or there might be some extra value in defining these via command-line flags (with default values as they are set now), so one could try to see how CBTree behaves with customized settings? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@928 PS1, Line 928: esy ? 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? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@938 PS1, Line 938: barrier nit: barriers? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@947 PS1, Line 947: variables nit: values http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@958 PS1, Line 958: for (;;) style nit for here and below: is it possible to use 'while (true)' to match the rest of the code in this file and in the Kudu project overall? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1024 PS1, Line 1024: Threads nit: threads http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1028 PS1, Line 1028: LOG_TIMING(INFO, Does it make sense to check for failures before starting the second phase of the scenario? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1029 PS1, Line 1029: $2 nit: $2 values ? http://gerrit.cloudera.org:8080/#/c/21447/1/src/kudu/tablet/cbtree-test.cc@1029 PS1, Line 1029: Threads nit: threads -- 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: 1 Gerrit-Owner: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 May 2024 23:26:13 +0000 Gerrit-HasComments: Yes
