Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-24 Thread Sravya Tirukkovalur 

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


Ship it!




Ship It!

- Sravya Tirukkovalur


On Oct. 24, 2016, 11:25 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 24, 2016, 11:25 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-24 Thread Hao Hao 

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

(Updated Oct. 24, 2016, 11:25 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 89892924839df8058ea82e7819973d576420f578 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9e9358b8bdcfb4177d0320da26739d990d287f09 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-12 Thread Alexander Kolbasov 

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


Ship it!




Just one small nit with logging.

- Alexander Kolbasov


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-12 Thread Alexander Kolbasov 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 237)


These two error messages are always indistinguishable. Can you reword them 
so that it is easy to see whether we got an exception or ID changed? It would 
be nice to be able to grep for specific word to see whether any of these 
happened.


- Alexander Kolbasov


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-12 Thread Sravya Tirukkovalur 

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



Can we add a comment that, this is a workaround until HIVE-14906 is fixed and 
Sentry starts using the new atomic snapshot API?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 204)


Make it needHiveSnapshot()?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 205)


In the new design, we would never be required to read the full snapshot 
from the HMSFollower isnt it? If so, can we remove this comment?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 209)


We are not stopping the HMS activity from Sentry. So may be reword it like: 
We need to make sure there were no HMS updates while retriving the snapshot?


- Sravya Tirukkovalur


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-12 Thread Li Li 

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


Ship it!




Ship It!

- Li Li


On Oct. 12, 2016, 7:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 12, 2016, 7:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-12 Thread Hao Hao 

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

(Updated Oct. 12, 2016, 7:15 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 89892924839df8058ea82e7819973d576420f578 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9e9358b8bdcfb4177d0320da26739d990d287f09 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-11 Thread Hao Hao 


> On Oct. 7, 2016, 12:56 a.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 77
> > 
> >
> > Shall we name this as needHiveSnapshot instead, to be more descriptive? 
> > And add a comment why this is set to false?

Make sense, will change the name.


> On Oct. 7, 2016, 12:56 a.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 206
> > 
> >
> > I think there are two different requirements here
> > 1. We will need to get Hive snapshot if the Notification ID we are 
> > requesting has been rolled over in the NotificationLog table of Hive.
> > 2. If we decide to store the current HMS snapshot in memory in Sentry 
> > deamon - At sentry deamon restart, we should load the full state from 
> > Sentry DB if it exists.
> > These two are very different and should be handled separately. It feels 
> > like we are confusing between two cases here.

Yeah, actaully we only store the current HMS snapshot in Sentry DB, so will 
change the TODO accordingly.


- Hao


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


On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 6, 2016, 10:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-11 Thread Li Li 


> On Oct. 11, 2016, 6:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 77
> > 
> >
> > I think we should set fullUpdateComplete to static, so that when the 
> > last HMSFollower update it to true, the next HMSFollower won't do full 
> > update again.

oops, I think I was wrong. You are right. For the ScheduledExecutorService, we 
do only create HMSFollower once and recall run method.


- Li


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


On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 6, 2016, 10:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-11 Thread Li Li 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 77)


I think we should set fullUpdateComplete to static, so that when the last 
HMSFollower update it to true, the next HMSFollower won't do full update again.


- Li Li


On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 6, 2016, 10:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-10 Thread Li Li 


> On Sept. 24, 2016, 11:59 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 215-216
> > 
> >
> > fullUpdateComplete variable is only used inside HMSFollower class - I 
> > would consider moving it inside and making it private. Since it is accessed 
> > by a single executor thread only there is no need to make it atomic 
> > (although this wouldn't hurt).
> 
> Hao Hao wrote:
> I do not think we shoud change from atomic to non-atomic. 
> fullUpdateComplete is a shared flag between main thread and HMSFollower. Once 
> fullUpdateComplete is true, which means full update is complete, HMSFollower 
> should not retrieve full update again.
> 
> Alexander Kolbasov wrote:
> Keeping it atomic is fine. But it seems that fullUpdateComplete isn't 
> actually shared - it is only used by HMSFollower thread. That's why I 
> suggested moving the variable inside HMSFollower class.
> 
> Hao Hao wrote:
> I see, will change it accordingly.

actually fullUpdateComplete is also used in SentryService as a centralized 
place to share with all HMSFollower threads. Though HMSFollowers are not 
sharing it at the same time as only 1 HMSFollower is running at certain time, 
it is a state shared by all HMSFollower threads. The new HMSFollower thread can 
decide whether need a full update or not, according to the value of 
fullUpdateComplete updated by the last HMSFollower thread.


- Li


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


On Oct. 6, 2016, 10:22 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 6, 2016, 10:22 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  89892924839df8058ea82e7819973d576420f578 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  9e9358b8bdcfb4177d0320da26739d990d287f09 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-06 Thread Hao Hao 

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

(Updated Oct. 6, 2016, 10:22 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 89892924839df8058ea82e7819973d576420f578 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9e9358b8bdcfb4177d0320da26739d990d287f09 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Hao Hao 

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

(Updated Oct. 4, 2016, 11:18 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 89892924839df8058ea82e7819973d576420f578 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9e9358b8bdcfb4177d0320da26739d990d287f09 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Hao Hao 


> On Sept. 23, 2016, 4:41 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 239-257
> > 
> >
> > Are there any established markers in the log for such events?
> 
> Hao Hao wrote:
> We did not have one before. Do you think it make sense to add one for it?
> 
> Alexander Kolbasov wrote:
> At a minimum we should be consistent within Sentry. If there are any 
> common guidelines for other HDFS products we may follow them. If not we 
> should just decide on something and stick to it.

Agree, I can file a jira for adding marker in the log and tracking the 
discussion there?


- Hao


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


On Oct. 4, 2016, 5:42 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 4, 2016, 5:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Hao Hao 


> On Sept. 24, 2016, 11:59 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 215-216
> > 
> >
> > fullUpdateComplete variable is only used inside HMSFollower class - I 
> > would consider moving it inside and making it private. Since it is accessed 
> > by a single executor thread only there is no need to make it atomic 
> > (although this wouldn't hurt).
> 
> Hao Hao wrote:
> I do not think we shoud change from atomic to non-atomic. 
> fullUpdateComplete is a shared flag between main thread and HMSFollower. Once 
> fullUpdateComplete is true, which means full update is complete, HMSFollower 
> should not retrieve full update again.
> 
> Alexander Kolbasov wrote:
> Keeping it atomic is fine. But it seems that fullUpdateComplete isn't 
> actually shared - it is only used by HMSFollower thread. That's why I 
> suggested moving the variable inside HMSFollower class.

I see, will change it accordingly.


- Hao


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


On Oct. 4, 2016, 5:42 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 4, 2016, 5:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Hao Hao 


> On Oct. 4, 2016, 6:44 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java,
> >  lines 130-135
> > 
> >
> > So this isn't actually a maximum count of retries since the code 
> > retries indefinitely. This is number of retries happening before the sleep. 
> > So I would ask whether we need it at all - why not just continue retrying 
> > (possibly sleeping a bit after each try)?

Good point, will remove the config here.


- Hao


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


On Oct. 4, 2016, 5:42 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 4, 2016, 5:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Alexander Kolbasov 


> On Sept. 24, 2016, 11:59 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 215-216
> > 
> >
> > fullUpdateComplete variable is only used inside HMSFollower class - I 
> > would consider moving it inside and making it private. Since it is accessed 
> > by a single executor thread only there is no need to make it atomic 
> > (although this wouldn't hurt).
> 
> Hao Hao wrote:
> I do not think we shoud change from atomic to non-atomic. 
> fullUpdateComplete is a shared flag between main thread and HMSFollower. Once 
> fullUpdateComplete is true, which means full update is complete, HMSFollower 
> should not retrieve full update again.

Keeping it atomic is fine. But it seems that fullUpdateComplete isn't actually 
shared - it is only used by HMSFollower thread. That's why I suggested moving 
the variable inside HMSFollower class.


> On Sept. 24, 2016, 11:59 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 215-216
> > 
> >
> > We can't throw exceptions from run() method since it will prevent it 
> > from ever running again.
> 
> Hao Hao wrote:
> During the process of fetching full update, I did not throw any exception 
> but just returned to the caller.

Your code doesn't, but there is existing code that does:

} catch (SentryInvalidInputException|SentryInvalidHMSEventException e) {
  throw new RuntimeException(e);
}


- Alexander


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


On Oct. 4, 2016, 5:42 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 4, 2016, 5:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-10-04 Thread Alexander Kolbasov 


> On Sept. 23, 2016, 4:41 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 239-257
> > 
> >
> > should it be an exception rather than return? Or return false to the 
> > caller to let it know that update fails.
> 
> Hao Hao wrote:
> Not sure if should throw exception or just return. Since the HMSfollower 
> is scheduled for execute periodically, I think it make sense if cannot get a 
> full snapshot this time can continue fetching next task?

Agreed. Since it is a periodic thing it doesn't help much to throw an excetion. 
It is a bit strange that HMSFollower doesn't handle its scheduling internally 
but requires callers to schedule it instead. But this is outside the scope for 
this fix.


> On Sept. 23, 2016, 4:41 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 239-257
> > 
> >
> > It seems that the whole fullUpdateCOmplete isn't used.
> 
> Hao Hao wrote:
> fullUpdateComplete is indicate a full snapshot is fetched and do not need 
> to get a full snapshot again.

I see. It is fine then.


> On Sept. 23, 2016, 4:41 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  lines 239-257
> > 
> >
> > Are there any established markers in the log for such events?
> 
> Hao Hao wrote:
> We did not have one before. Do you think it make sense to add one for it?

At a minimum we should be consistent within Sentry. If there are any common 
guidelines for other HDFS products we may follow them. If not we should just 
decide on something and stick to it.


- Alexander


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


On Oct. 4, 2016, 5:42 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Oct. 4, 2016, 5:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-24 Thread Alexander Kolbasov 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 215 - 216)


fullUpdateComplete variable is only used inside HMSFollower class - I would 
consider moving it inside and making it private. Since it is accessed by a 
single executor thread only there is no need to make it atomic (although this 
wouldn't hurt).



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 215 - 216)


We can't throw exceptions from run() method since it will prevent it from 
ever running again.


- Alexander Kolbasov


On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 6:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-24 Thread Alexander Kolbasov 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 238 - 257)


I think that we should set currentEventId to eventIDBefore.


- Alexander Kolbasov


On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 6:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-23 Thread Li Li 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 214)


how about adding 'TODO: expose time used for full update in the metrics' ?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 257)


shall we update currentEventID with eventIDBefore(or 
eventIDAfter).getEventId(),
so that client will process notification events from eventIDBefore + 1 ?


- Li Li


On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 6:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-23 Thread Alexander Kolbasov 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 75 - 77)


point-in-time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 93 - 95)


Why do you need fullUpdateConmplete variable at all and why is it atomic?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 215 - 216)


This TODO seems obsolete - isn't what you are doing in the code below?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 217 - 237)


Please add a comment describing state of affairs when the code doesn't 
converge. What happens in this case?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 217 - 237)


I think for HMS it is called Notification ID, so it is better to use 
NotificationID name in the log to be consistent with HMS terminology



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


Are there any established markers in the log for such events?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


Please also add the value of event ID here - it may be useful to debug from 
Hive side.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


Same comments for failure to get full update



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


should it be an exception rather than return? Or return false to the caller 
to let it know that update fails.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


It seems that the whole fullUpdateCOmplete isn't used.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (lines 239 - 257)


Please put a message in the log telling at which ID the full snapshot was 
taken



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 (lines 130 - 134)


Please add a comment explaining what happens when the max is reached.


- Alexander Kolbasov


On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 6:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-23 Thread Li Li 

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


Ship it!




Ship It!

- Li Li


On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 6:15 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-23 Thread Hao Hao 

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

(Updated Sept. 23, 2016, 6:15 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 894fcc966b511ccf309599fd10960f9a11ae8e96 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 abc3f58d21bb774427a34399b6e9f51a37ba51db 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-22 Thread Li Li 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 217)


require -> requires



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 229)


From the comment, seems you want to update eventIDBefore before 
fetchFullUpdate() in the while loop



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 252)


I think we don't need 'currentRetries == maxRetriesForHMSSnapshot' here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 257)


why set fullUpdateComplete to false here?
Seems fullUpdateComplete is always false now.


- Li Li


On Sept. 23, 2016, 12:50 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 23, 2016, 12:50 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-22 Thread Hao Hao 


> On Sept. 22, 2016, 12:39 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java,
> >  line 212
> > 
> >
> > Is the TODO: read currentEventID still relevant?

Yeah, in the case when Sentry DB already stored a full snapshot need to read 
from DB. Will update the comment.


- Hao


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


On Sept. 22, 2016, 8:39 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 22, 2016, 8:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-22 Thread Li Li 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 206)


return if client == null, and throw / log the exception



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 216)


could we change the currentEventID to a different name, since we have a var 
with the same name in line 64



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 219)


I think we should give up if fetchFullUpdate() fails, since 
FullUpdateInitializer also has retry, and retry here is just to fetch the 
latest snapshot.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 (line 220)


Shall we update currentEventID before retry?
Shall we use !equals instead of != ?


- Li Li


On Sept. 22, 2016, 8:39 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> ---
> 
> (Updated Sept. 22, 2016, 8:39 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1463: Ensure HMS point-in-time snapshot consistency
> 
> The implemented logic is:
> 1. Read current HMS notification ID_initial
> 2. Read HMS metadata state
> 3. Read current notification ID_new
> 4. If ID_initial != ID_new then discard the current state and goto 1.
>  
> Use configurable property: sentry.hms.snapshot.retries.max.count for max 
> number of retry.
> 
> Change-Id: I7590076b875bd97b2fb340008926ea5995896d72
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  894fcc966b511ccf309599fd10960f9a11ae8e96 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
> 
> Diff: https://reviews.apache.org/r/52138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-21 Thread Hao Hao 

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

(Updated Sept. 21, 2016, 11:47 p.m.)


Review request for sentry and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.
 
Use configurable property: sentry.hms.snapshot.retries.max.count for max number 
of retry.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 894fcc966b511ccf309599fd10960f9a11ae8e96 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 abc3f58d21bb774427a34399b6e9f51a37ba51db 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao

Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot consistency

2016-09-21 Thread Hao Hao 

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

Review request for sentry and Sravya Tirukkovalur.


Repository: sentry


Description
---

SENTRY-1463: Ensure HMS point-in-time snapshot consistency

The implemented logic is:
1. Read current HMS notification ID_initial
2. Read HMS metadata state
3. Read current notification ID_new
4. If ID_initial != ID_new then discard the current state and goto 1.

Change-Id: I7590076b875bd97b2fb340008926ea5995896d72


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 894fcc966b511ccf309599fd10960f9a11ae8e96 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 abc3f58d21bb774427a34399b6e9f51a37ba51db 

Diff: https://reviews.apache.org/r/52138/diff/


Testing
---


Thanks,

Hao Hao