> On Dec. 16, 2016, 7:05 p.m., Vadim Spector wrote: > > It is probably ok, but ... > > a) Did you investigate why service stayed alive? Perhaps, some knowledge > > about the implementation and its defficiencies to be gained there. Some > > unaccounted runaway threads? Or are we unable to termonaie some threads? > > b) System.exit() means that Sentry service better be the only thing running > > in JVM. Which I presume is the case now. Are we ok with making that a > > requirement from now on? > > Alexander Kolbasov wrote: > For a) please see SENTRY-1526. The service stayed alive because of the > deadlock. > b) I don't understand the question - what else can run in this JVM? This > is a Sentry Server that we are trying to kill. > > Vadim Spector wrote: > a) Can the deadlock be fixed instead? SENTRY-1526 seem to suggest that > the choice of synchronization objects can be changed to prevent the deadlock. > It still does not explain why we need to use System.exit(). > b) Architecturally, there is nothing insane about running Sentry Service > embedded inside some kind of container in the future. > We may not need it now, but the proposed implementation makes this > impossible moving forward, therefore my concern. > > Misha Dmitriev wrote: > Regarding (a). This is not a classic deadlock (which means two threads > holding two separate locks). Here we have something simpler: thread A > preventing thread B from proceeding by holding a single lock unnecessarily, > by calling synchronized method waitOnFutures(). If you look at its code and > how it's used, you will immediately see that this method just doesn't need to > be synchronized. Sasha fixed that by removing the whole waitOnFutures() > method altogether and moving its code (a single line) into the only caller of > waitOnFutures().
Yes, I saw it. So, if it's fixed, I still don't understand why we still need System.exit(), that's my question. - Vadim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54808/#review159478 ----------------------------------------------------------- On Dec. 16, 2016, 7:23 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54808/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2016, 7:23 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > SENTRY-1526 Sentry processed stays alive after being killed > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 578364933a3cdcf6c142b836360a83d322fe5c11 > > Diff: https://reviews.apache.org/r/54808/diff/ > > > Testing > ------- > > Now process successfully dies after hitting ^C. > > > Thanks, > > Alexander Kolbasov > >
