You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2016/12/16 07:23:31 UTC
Review Request 54808: SENTRY-1526 Sentry processed stays alive after
being killed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/
-----------------------------------------------------------
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
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/#review159475
-----------------------------------------------------------
Ship it!
Ship It!
- Vamsee Yarlagadda
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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Misha Dmitriev <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/#review159415
-----------------------------------------------------------
Ship it!
Ship It!
- Misha Dmitriev
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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/#review159488
-----------------------------------------------------------
Ship it!
- kalyan kumar kalvagadda
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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Alexander Kolbasov <ak...@gmail.com>.
> 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?
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.
- Alexander
-----------------------------------------------------------
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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Vadim Spector <vs...@cloudera.com>.
> 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.
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.
- 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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Misha Dmitriev <mi...@cloudera.com>.
> 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.
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().
- Misha
-----------------------------------------------------------
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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Vadim Spector <vs...@cloudera.com>.
> 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
>
>
Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive
after being killed
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54808/#review159478
-----------------------------------------------------------
Ship it!
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?
- Vadim Spector
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
>
>