Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Haley Reeve via Review Board]

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


Ship it!




Ship It!

- Haley Reeve


On Nov. 27, 2018, 9:34 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 27, 2018, 9:34 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2018, 9:34 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 27, 2018, 9:34 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

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

(Updated Nov. 27, 2018, 9:34 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Bugs: SENTRY-2464
https://issues.apache.org/jira/browse/SENTRY-2464


Repository: sentry


Description
---

On starting UpdatableCache update thread, if blockUntilFirstReload value is set 
to true, an exception thrownwill cause the cache to never initialize and 
services using this cache will stop


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 0dd7b4a61 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

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

(Updated Nov. 27, 2018, 9:31 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Changes
---

Post feedback


Bugs: SENTRY-2464
https://issues.apache.org/jira/browse/SENTRY-2464


Repository: sentry


Description
---

On starting UpdatableCache update thread, if blockUntilFirstReload value is set 
to true, an exception thrownwill cause the cache to never initialize and 
services using this cache will stop


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 0dd7b4a61 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Sergio Pena via Review Board]

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


Fix it, then Ship it!




It looks good, just add to the warning message that the cache will be reloaded 
later in the next time interval or something. The reason is that if a user 
finds a warning messge, they expect an answer of what to do, right?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Lines 140 (patched)


If this is a warning, should the user expect to do something if tihs 
message appears on the log? If not, perhaps you should add to the message that 
the cache will be reloaded in the next refresh interval (in X seconds?).


- Sergio Pena


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

> On Nov. 26, 2018, 7:44 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
> > Line 37 (original), 37 (patched)
> > 
> >
> > should this be volatile  or atomic?
> 
> Arjun Mishra wrote:
> Lina, can you explain why adding volatile here is needed? For each Kafka 
> broker there is a single KafkaAuthzBinding instance. So 
> UpdatableCache#startUpdateThread will be called once by a KafkaBroker. This 
> means there will be a reloadData thread for each Kafka broker.

Is it possible for more than one threads to access that single 
KafkaAuthzBinding instance? If so, we should define "initialized" as volatile 
to handle concurrency correctly. Otherwise, you don't need to do so


- Na


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


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

> On Nov. 26, 2018, 7:44 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
> > Line 37 (original), 37 (patched)
> > 
> >
> > should this be volatile  or atomic?
> 
> Arjun Mishra wrote:
> Lina, can you explain why adding volatile here is needed? For each Kafka 
> broker there is a single KafkaAuthzBinding instance. So 
> UpdatableCache#startUpdateThread will be called once by a KafkaBroker. This 
> means there will be a reloadData thread for each Kafka broker.
> 
> Na Li wrote:
> Is it possible for more than one threads to access that single 
> KafkaAuthzBinding instance? If so, we should define "initialized" as volatile 
> to handle concurrency correctly. Otherwise, you don't need to do so

It isn't since there is 1 KafkaAuthzBinding for every Kafka broker. Even if it 
was this change should be handled in a different ticket right?


- Arjun


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


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

> On Nov. 26, 2018, 7:44 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
> > Line 37 (original), 37 (patched)
> > 
> >
> > should this be volatile  or atomic?

Lina, can you explain why adding volatile here is needed? For each Kafka broker 
there is a single KafkaAuthzBinding instance. So 
UpdatableCache#startUpdateThread will be called once by a KafkaBroker. This 
means there will be a reloadData thread for each Kafka broker.


- Arjun


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


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Line 37 (original), 37 (patched)


should this be volatile  or atomic?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Line 145 (original), 149 (patched)


It is better to make initialized volatile


- Na Li


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

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

(Updated Nov. 26, 2018, 6:14 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Bugs: SENTRY-2464
https://issues.apache.org/jira/browse/SENTRY-2464


Repository: sentry


Description (updated)
---

On starting UpdatableCache update thread, if blockUntilFirstReload value is set 
to true, an exception thrownwill cause the cache to never initialize and 
services using this cache will stop


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 0dd7b4a61 


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


Testing
---


Thanks,

Arjun Mishra

Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

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

Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Bugs: SENTRY-2464
https://issues.apache.org/jira/browse/SENTRY-2464


Repository: sentry


Description
---

On starting UpdatableCache update thread, if blockUntilFirstReload value is set 
to true, thrown an exception will cause the cache to never initialize and 
services using this cache will stop


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 0dd7b4a61 


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


Testing
---


Thanks,

Arjun Mishra