Alexey Serbin has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 27: (9 comments) Will send a new version in a moment. 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? Done Line 2173: const string LONG_STRING(kBufferSizeBytes + 1, 'x'); > Nit: kLongString. Done Line 2206: const string LONG_STRING(kBufferSizeBytes / 2, 'x'); > Nit: kLongString. Done Line 2378: thread monitor(monitor_func); > How about: I was under impression that elsewhere in the code we have those intermediate variables for clarity. OK, but it seems this time not having that would be an advantage. Will do. I was thinking about the lambda as well, but preferred to keep it as is to be in line with the MonitorSessionBufferSize counterpart (as you noticed). I would prefer to keep it as is. Do you insist on using a lambda here? 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. Done 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. Done PS27, Line 2498: BUFFER_SIZE > Nit: replace with kBufferSizeBytes. Below too. Done 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 b Done 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. Done -- 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
