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 <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