Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Haley Reeve via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212610
---


Ship it!




Ship It!

- Haley Reeve


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212387
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

> On Jan. 25, 2019, 9:01 p.m., kalyan kumar kalvagadda wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
> > Lines 150 (patched)
> > 
> >
> > You are adding 250 msec delay. Is it good enough?
> > 
> > There are few tests that have sentryServers.size()> 1,  so we should be 
> > good even with higher delay.
> > 
> > 
> > Based on your tests if you think that this delay is good enough, i'm 
> > fine.
> 
> Na Li wrote:
> Higher delay is not necessarily good to avoid deadlock. The table 
> creation happens only when HMS followers gets notification and create table, 
> which happens after all sentry services started. So starting services with 
> delay itself does not fix the issue. What really fixes the issue is the space 
> out when HMS follower threads run. Those threads run periodically. The 
> biggest space between two threads is half of the cycle. The biggest space for 
> N threads is cycle/N.
> 
> I can make the code change to handle thread number > 2.

I have changed the code, so the first instance of sentry server does not wait 
(this avoid unnecessary wait in test). Only the following instances will wait 
for certain time to avoid creating table at the same time.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212353
---


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/
---

(Updated Jan. 28, 2019, 4:19 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


Bugs: sentry-2491
https://issues.apache.org/jira/browse/sentry-2491


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


Diff: https://reviews.apache.org/r/69840/diff/3/

Changes: https://reviews.apache.org/r/69840/diff/2-3/


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/
---

(Updated Jan. 25, 2019, 9:46 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


Bugs: sentry-2491
https://issues.apache.org/jira/browse/sentry-2491


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


Diff: https://reviews.apache.org/r/69840/diff/2/

Changes: https://reviews.apache.org/r/69840/diff/1-2/


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

> On Jan. 25, 2019, 9:01 p.m., kalyan kumar kalvagadda wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
> > Lines 150 (patched)
> > 
> >
> > You are adding 250 msec delay. Is it good enough?
> > 
> > There are few tests that have sentryServers.size()> 1,  so we should be 
> > good even with higher delay.
> > 
> > 
> > Based on your tests if you think that this delay is good enough, i'm 
> > fine.

Higher delay is not necessarily good to avoid deadlock. The table creation 
happens only when HMS followers gets notification and create table, which 
happens after all sentry services started. So starting services with delay 
itself does not fix the issue. What really fixes the issue is the space out 
when HMS follower threads run. Those threads run periodically. The biggest 
space between two threads is half of the cycle. The biggest space for N threads 
is cycle/N.

I can make the code change to handle thread number > 2.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212353
---


On Jan. 25, 2019, 9:30 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 25, 2019, 9:30 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/1/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212353
---


Fix it, then Ship it!




Wait for the test to complete.


sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
Lines 150 (patched)


You are adding 250 msec delay. Is it good enough?

There are few tests that have sentryServers.size()> 1,  so we should be 
good even with higher delay.

Based on your tests if you think that this delay is good enough, i'm fine.


- kalyan kumar kalvagadda


On Jan. 25, 2019, 7:57 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 25, 2019, 7:57 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, HaleyHH HaleyHH, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/1/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/#review212350
---


Ship it!




Ship It!

- Arjun Mishra


On Jan. 25, 2019, 7:57 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 25, 2019, 7:57 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, HaleyHH HaleyHH, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/1/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69840/
---

Review request for sentry, Arjun Mishra, HaleyHH HaleyHH, and kalyan kumar 
kalvagadda.


Bugs: sentry-2491
https://issues.apache.org/jira/browse/sentry-2491


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


Diff: https://reviews.apache.org/r/69840/diff/1/


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li