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

[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

[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
> 
>

Question about SentryStore/DelegateSentryStore

[Alexander Kolbasov]

There is some strangeness in the way SentryStore/DelegateSentryStore is
implemented.

DelegateSentryStore implements SentryStoreLayer interface, but SentryStore
doesn't. It looks like SentryStore should implement this as well.

Any thoughts on this?

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

[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
> 
>

Review Request 52795: SENTRY-1499: Add feature flag for using NotificationLog

[Hao Hao]

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

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


Repository: sentry


Description
---

SENTRY-1499: Add feature flag for using NotificationLog

1. Keep two SentryMetastorePostEventListeners. One for using notificationlog, 
another for not.
2. Add feature flag for using HMSFollower and SentryMetastorePostEventListeners 
with notification log.

Change-Id: I46a11d27ae0159a735b6049e9b7c78216d0e5346


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
 75190c1805162e11baf7c4553668693e4e6bb3c4 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerNotificationLog.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9e9358b8bdcfb4177d0320da26739d990d287f09 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 f98ebd135a383ff090b1b204cc62644403310df7 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 61b24fa0e9f221b75ae343fc534b4d82fbce272a 

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


Testing
---


Thanks,

Hao Hao

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

[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 52675: Create a sentry scale test tool to add various objects and privileges into Sentry and HMS.

[Li Li]

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java
 (line 33)


Why make it non final?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java
 (line 60)


how about just using: val = System.getProperty(hiveVar, new 
Configuration().get(hiveVar)) ?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java
 (line 69)


seems it may not be system property, see line 63?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
 (line 42)


run -> running ?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
 (line 294)


db ++ -> db++ ?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
 (line 309)


tb ++ -> tb++ ?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/TestTools.java
 (line 68)


do we need to add 'else' for all other cases (print help)?


Better ask someone else to do a code review since I am not familiar with test 
framework.

- Li Li


On Oct. 11, 2016, 4:29 a.m., Anne Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52675/
> ---
> 
> (Updated Oct. 11, 2016, 4:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Li Li, and Sravya 
> Tirukkovalur.
> 
> 
> Bugs: SENTRY-1497
> https://issues.apache.org/jira/browse/SENTRY-1497
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Specify the scale numbers like databases, tables, views, partitions, columns, 
> uris, privileges, role, and groups in a config file, the tool can create such 
> volume of data in Sentry and HMS databases. To speed up test, it can also do 
> the task parallelly.
> 
> 
> Diffs
> -
> 
>   sentry-tests/sentry-tests-hive/pom.xml 
> a2512ee3919a3958425f4ab74b178d02e0402315 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java
>  90713b1aaa688808859e670c8799f8e5be2d6d26 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/TestTools.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/sentry_scale_test_config.xml
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/scripts/scale-test/create-many-dbs-tables.sh
>  dcdddeb95a896ca8470d0b994f5460531e34d113 
> 
> Diff: https://reviews.apache.org/r/52675/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anne Yu
> 
>

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

[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