David Ribeiro Alves has posted comments on this change.

Change subject: async background flush provision for C++ client

Patch Set 2:


overall I think this needs more tests. Since the flusher is a thread, likely 
the unit tests will also have to use threads to:
- make sure limits are not exceeded
- make sure that operations are in fact buffered up to a point
- Test the multiple triggers of a flush.

Beyond the the unit tests, I think this also needs an integration test that 
stresses it all.

Commit Message:

Line 7: async background flush provision for C++ client
nit: capitalize (Async)

PS2, Line 7: provision
nit: remove provision

PS2, Line 10: is
nit: avoid using the passive voice. i.e. "KuduSession starts a background 
flusher thread" not a thread is started

PS2, Line 13: virtual
what's a virtual buffer?

Line 16: The flush criteria is based on buffer size for not-yet-scheduled
I'm having trouble parsing this sentence.

PS2, Line 17: operatations
nit: type

PS2, Line 20: operatations
nit: typo

PS2, Line 23: KUDU
Mention this issue in the commit message title.

File src/kudu/client/batcher.cc:

Line 267: #ifndef NDEBUG
spurious change

Line 383: 
spurious change

Line 553:   //        since the other time it's called in KuduSession::Apply()
nit: rephrase. How about:
Optimize and call write_op->SizeInBuffer() only once. We're currently calling 
it twice, here and in KuduSession::Apply()

File src/kudu/client/batcher.h:

Line 109:   // Return number of bytes used for batcher buffer
nit: "Returns the"
nit: period at the end os sentences/

Line 114:   // Compute in-buffer size for the given write operation
nit: Computes
nit: period

PS2, Line 216: //
no need for this comment

File src/kudu/client/client-test.cc:

Line 1991: // return an error with session running in any supported flush mode
nit: period

PS2, Line 1993: buffer_space_bytes_limit
style: kBufferSpaceBytesLimit or BUFFER_SPACE_BYTES_LIMIT

PS2, Line 1993: static
not need for static.

PS2, Line 1994: static

Line 1995:   static const KuduSession::FlushMode modes[] = {

Line 2001:   for (const auto mode: modes) {
isn't 'auto' already const ?

Line 2007:     ASSERT_TRUE(s.IsIncomplete()) << "got unexpected status: " << 
nit: capitalize Got

Line 2014:   // Applying a bunch of small rows without a flush should not 
result in
this sentence is huge and hard to read. please break it down.

Line 2016:   // since there is a flow control which makes Session::Apply() 
block and wait
what: "a flow control"?

Line 2018:   {
no need for the extra { scope

Line 2023:       ASSERT_OK(ApplyInsertToSession(session.get(), client_table_, 
1, 1, "x"));
How this testing that Apply() is eventually blocking, or making sure that the 
buffer value is not surpassed for that matter?

Line 2029: // operations are put into the queue flushed after some time.
you mean : "into the queue and flushed" right?

Line 2030: // The exact timeout interval is implementation-dependent,
which timeout interval?

Line 2031: // but 100 ms is a good upper limit anyways.

Line 2033:   {
no need for the extra {

Line 2041:     SleepFor(MonoDelta::FromMilliseconds(100));
loop instead of wait, otherwise this will be brittle in jenkins

File src/kudu/client/client.cc:

Line 705:           "Cannot change flush mode when writes are buffered");
nit period

Line 707:   data_->SetFlushMode(m);
would it simplify things to now allow the flush mode to change if there is a 
flusher thread? if not would it make sense to have this stop the thread (if the 
new mode is different) and wait for it to finish before allowing to change the 
mode? seems like there is some complexity attached to the fact that we allow 
this to change at anytime. simplifying the possible states there would be good

Line 728:   return data_->SetBufferBytesLimit(size);
why not use the same method name?

File src/kudu/client/client.h:

Line 678:     // The Flush() call can be used to block until the latest batch 
is sent.
maybe add/replace: "a batch is sent and the buffer has available space again"

File src/kudu/client/session-internal.cc:

Line 48:   // intentionally left empty
no ned for this, you can also join the brackets in the line above, i.e. {}

Line 53:     WARN_NOT_OK(Stop(), "unable to stop AUTO_FLUSH_BACKGROUND thread");
capitalize. Also a WARN? what happens if this fails?

Line 58:   CHECK(!thread_) << "has already started";
complete the sentence

Line 69:   while (true) {
Overall I think this logic can be simplified

You can have a simple_spinlock that protects the relevant fields (like the 
callback or is_shutdown_). When the callback is set, it means that an explicit 
flush was required (the point is to get rid of a couple of bools there, but 
would understand keeping one).

Finally you can replace the condition variables with a CountDownLatch (which 
cvs internally).

each time you get a condition that makes the loop progress or exit use 
latch_->CountDown()  and you can wait on the latch where relevant.

PS2, Line 75: ||
nit: style, move this to the line below, same elsewhere

Line 85:         // high watermark is 1/2 of the buffer space limit for 
non-flushed data
can you use a flag for the over_watermark that you set as a percentage of the 
buffer limit?

Line 98:         do_explicit_flush = true;
these two variables are confusing.

Line 114:       sp::shared_ptr<KuduSession> session(weak_session_.lock());
why not just increase the ref count?

Line 138:   is_explicit_flush_ = true;
could you get rid of this bool and just call the cb if it is set?

Line 149:       buffer_bytes_limit_(7 * 1024 * 1024),  // TODO: clarify on this
use a FLAG for this

Line 160: void KuduSession::Data::Init(const shared_ptr<KuduSession>& session) {
be consistent. sometimes you use shared_ptr, other time sp:shared_ptr and other 
times client::sp::shared_ptr, you should use the least amount possible of 

Line 167:   WARN_NOT_OK(flusher_->Start(),
shouldn't this return an error to the caller

Line 198:   // Both flush and flow control logic need to know on buffer limit 
nit: remove "on"
more importantly overall I think you should only allow to set a buffer limit 
before you start writing.

Line 206: Status KuduSession::Data::CheckAgainstBufferLimit(size_t 
required_size) const {
nit: remove "Against"

Line 222:     WARN_NOT_OK(flusher_->ForceFlush(cb), "failed to wake background 
isn't this serious enough that the caller should get an error?

Line 335:     // because the background flusher could trigger spurios flush 

File src/kudu/client/session-internal.h:

PS2, Line 53: is set on currently
I don;t think you need to mention what the caller will pass as a flush mode.

Line 58:   // Set limit on buffer space consumed by buffered write operations
nit period here and elsewhere

Line 65:   // calling the callback with the flush result
nit period

Line 105:   void LessBufferedBytes(int64_t bytes_used);
less seems to imply comparison. Can you rename to DecreaseBufferedBytes

Line 107:   // Check if the specified size fits buffer given its current size 
period at the end of this sentence and all the other ones.

Line 110:   // Mutex for the condition variables below
this seems to be used for more than the ConditionVariables.

Line 129:   class BackgroundFlusher {
move the inner class above the other private methods and fields.

Line 137:     // initialize the object and start the thread
capitalize here and elsewhere

Line 151:     client::sp::weak_ptr<KuduSession> weak_session_;
why isn't this hanging on to the KuduSession. What happens if it gets deleted 
while this thread is working (remember the user controls the lifetime if you 
don't increase the ref count)

PS2, Line 155: consequitive
nit typo

Line 158:     // whether the thread is shutdown (guarded by cond_mutex_)
nit: capitalize here and elsewhere

Line 169:   }; // class KuduSession::Data::Flusher
we don't usually use class closing comments

To view, visit http://gerrit.cloudera.org:8080/3668
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfde05bef342db24990c6e3da3b0270c3bb37a9d
Gerrit-PatchSet: 2
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: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to