Alexey Serbin has posted comments on this change.

Change subject: client/sample.cc: fixed a couple of crashes
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3685/1//COMMIT_MSG
Commit Message:

Line 11: while logging some messages from terminating reactor threads.
> is there another way we can fix this without forcing callers to uninstall t
I could think of making the logger callback static with lifespan longer than 
those objects in the main() scope.  However, I'm not sure that will guarantee 
the desired behavior: I'm not sure there is a guarantee the reactor threads 
will be dead at time destruction of that static object.

Another thing I'm thinking is to create a wrapper object which would install 
the logger callback in constructor and un-install the callback in its 
destructor (no exceptions in case of failures, though -- only messages to 
std::cerr).

But those are about working around the real issue of shutting down reactor 
threads independently in the background.  Probably, the destructor of 
KuduClient should await for completion of those reactor threads instread.


Line 11: while logging some messages from terminating reactor threads.
> As with the below, any ideas as to why the precommit test didn't experience
There is a catch: I modified the sample.cc to run with different parameters.  
That exposed this bug.  I built the source in debug configuration.  If needed, 
I can send you the patch.

Basically, I played around the sample.cc building different scenarios to run 
against the original client library code and code with my recent modifications. 
 The issues fixed in the patch appear even in client library with no 
AUTO_BACKGROUND_FLUSH support.


Line 13: Fixed issue with an attempt to access non-existing element
> hmm, the bug fix looks reasonable, but I'm curious why we don't see this cr
Please see the response for the prior comment: in essence, I run modified 
sample.cc and that exposed the bug.


Line 13: Fixed issue with an attempt to access non-existing element
> I'm also surprised. front() on an empty vector is undefined behavior, so ma
I found that going through different scenario in sample.cc, basically modified 
the sample.cc code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5fa3b812e6402a113bf5e432a3a451dc4cc3735
Gerrit-PatchSet: 1
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to