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 <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to