Adar Dembo has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 27: (9 comments) Almost there. http://gerrit.cloudera.org:8080/#/c/3952/27/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS27, Line 95: sing std::thread; : using std::bind; : using std::function; : using std::for_each; Nit: can you order this list so it's alphabetical? Line 2173: const string LONG_STRING(kBufferSizeBytes + 1, 'x'); Nit: kLongString. Line 2206: const string LONG_STRING(kBufferSizeBytes / 2, 'x'); Nit: kLongString. Line 2378: thread monitor(monitor_func); How about: thread monitor(bind(&ClientTest::MonitorSessionBatchersCount, session.get(), &monitor_run_ctl, &monitor_max_batchers_count)); That is, combine the two statements from above. This avoids exposing the "messy" std::function signature, though I suppose you could also elide it by using "auto" as the type. Ideally you'd use a lambda here. I think MonitorSessionBatchersCount is only used in this test, so maybe make it a lambda instead? MonitorSessionBatchersSize is used in two places, so you can't use it there. PS27, Line 2455: function<void()> monitor_func( : bind(&ClientTest::MonitorSessionBufferSize, : session.get(), &monitor_run_ctl, : &monitor_max_buffer_size)); : thread monitor(monitor_func); Likewise here, combine or use auto to elide the std::function syntax. PS27, Line 2493: function<void()> monitor_func( : bind(&ClientTest::MonitorSessionBufferSize, : session.get(), &monitor_run_ctl, &monitor_max_buffer_size)); : thread monitor(monitor_func); And here. PS27, Line 2498: BUFFER_SIZE Nit: replace with kBufferSizeBytes. Below too. Line 2501: for (size_t i = 3; i < 256; ++i) { Nit: maybe store 256 in a constant like kRowNum and reuse it in EXPECT_EQ below? PS27, Line 2533: function<void()> monitor_func( : bind(&ClientTest::MonitorSessionBufferSize, : session.get(), &monitor_run_ctl, &monitor_max_buffer_size)); : thread monitor(monitor_func); Combine or auto. -- To view, visit http://gerrit.cloudera.org:8080/3952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8 Gerrit-PatchSet: 27 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
