Dan Burkert has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 9:


File src/kudu/client/batcher.h:

Line 64:   // Takes a reference on error_collector. Creates a weak_ptr to 
Could you update this comment, especially WRT the error_collector?  I can't 
tell if the intent is that this constructor takes ownership of the error 
collector, or if it's borrowed.

Line 115:   static int64_t GetOperationSizeInBuffer(KuduWriteOperation* 
This doesn't seem too useful since it just calls write_op->SizeInBuffer().  Any 
reason to prefer having it on the batcher?

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

PS9, Line 555: session
the session

PS9, Line 557: 10x multiplier
10x seems like a lot of leeway.  Could we get away with 2 or 3x?  I would 
consider it a bug if we routinely took 10x longer than the deadline.

PS9, Line 557: maximum 
the maximum

PS9, Line 2221: Status s;
looks like everywhere this is used the call could instead be wrapped in 
ASSERT_OK.  I think that also tends to give better error messages on failure.

PS9, Line 2307: scenrio
'scenario' here and below

PS9, Line 2478: previos

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

Line 36: DEFINE_int32(client_buffer_bytes_limit, 7 * 1024 * 1024,
Remove this and use SetMutationBufferSpace instead.

Line 49: DEFINE_int32(client_buffer_flush_watermark_pct, 75,
Instead of a gflag, this should be an option on KuduSession, like 
SetMutationBufferSpace.  This also mirrors the equivalent Java options: 

Line 61: DEFINE_int32(client_buffer_flush_timeout_ms, 10,
Same here, this should be an option on KuduSession.  

PS9, Line 82: :
wrap init list.

Line 115:       messenger->ScheduleOnReactor(
It's not clear to me why this needs to be scheduled on a reactor thread.  I get 
that you don't want to block the user thread on the apply call, but if 
BackgroundFlusher::Run blocks, it's not appropriate to be running on the 
reactor thread anyway.

PS9, Line 154:   if (PREDICT_FALSE(!status.ok())) {
             :     return;
             :   }
             :   // Check that the session is still alive to access the data 
             :   sp::shared_ptr<KuduSession> session(weak_session.lock());
             :   if (PREDICT_FALSE(!session)) {
             :     return;
             :   }
             :   // Flush mode could change during the operation.  If current 
             :   // is no longer AUTO_FLUSH_BACKGROUND, then stop re-scheduling 
the task.
             :   if (PREDICT_FALSE(data_->flush_mode_ != 
             :     return;
             :   }
             :   // Detach the batcher, schedule its flushing,
             :   // and install a new one to accumulate incoming write 
             :   data_->NextBatcher(weak_session, 0, nullptr);
Looks like this might be able to call Run instead of duplicating?

PS9, Line 205: MutexLock
prefer to use std::lock_guard if possible.

PS9, Line 392: int64_t* op_raw_size
is this still necessary now that the size is cached?

Line 421:       flow_control_cond_.Wait();
Is a flush forced somewhere in this situation, or is it just waiting for the 
periodic flush?

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


Line 126:     ~BackgroundFlusher();
I think this can use the default keyword instead of defining the no-op dtor in 

Line 136:     void CheckAndRun(const std::shared_ptr<rpc::Messenger>& messenger,
Could you doc this?  I can't tell exactly what it's for, or why it chooses to 
run the flush on the reactor thread with no wait.

PS9, Line 151: KuduSession::Data* const
It looks like all of the methods on the background flusher that do any work 
take a weak_ptr.  I can't quite follow why this raw ptr is held, and the 
methods take the weak_ptr passed in.  Could a weak_ptr instead be passed in as 
part of the ctor and stored in a field, and then the methods above could use 
that?  I think that would also remove the dtor ordering thing below which is 
pretty fragile, since weak_ptrs can be checked before use.

PS9, Line 159: write operation
the write operation

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: 9
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to