[kudu-CR] client/sample.cc: fixed a couple of crashes

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] client/sample.cc: fixed a couple of crashes

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(2 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
As with the below, any ideas as to why the precommit test didn't experience 
this?

As for a less onerous fix, log_cb could be declared globally so that it doesn't 
go out of scope at the end of main(). That's probably what "real users" would 
do anyway.


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
I'm also surprised. front() on an empty vector is undefined behavior, so maybe 
the precommit compilers elide the entire statement, and Alexei has been using 
one that doesn't?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] client/sample.cc: fixed a couple of crashes

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

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


Patch Set 1:

(2 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 the 
log callback? it seems a little onerous to make users deal with this.


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 crash 
always in our precommit test runs?


-- 
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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes