Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-06 Thread Na Li via Review Board 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Lines 125 (patched)


Should you update the following condition for 
DBUpdateForwarder.getAllUpdatesFrom()?

Replace
if (seqNum > curSeqNum) {
  // No new notifications were processed.
  return Collections.emptyList();
}

 With
 if (seqNum > curSeqNum) {
 if(seqNum = curSeqNum + 1) {
  // No new notifications were processed.
  return Collections.emptyList();
  } else {
   // perm change has been reset, get full permsnapshot 
   ListfullImage = retrieveFullImage();
//Only log if we have received full image
if( !fullImage.isEmpty()) {
  LOGGER.info("({}) A newer full update with image number {} was 
found and being sent to HDFS",
  retrieverType, curImgNum);
}
return fullImage;
  }
}

 Once you clear the perm change, the seqNum from Name Node will be larger 
than curSeqNum. The correct behavior should be to send full snapshot of 
permission to Name Node after you clear the perm changes.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 596 (patched)


This method is not just for test, called in HMSFollower line 122. Why do 
you have this comment "the method only for test"?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 615 (patched)


it is called in HMSFollower, why comments "the method only for test"?


- Na Li


On Feb. 6, 2018, 7:06 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65533/
> ---
> 
> (Updated Feb. 6, 2018, 7:06 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2115
> https://issues.apache.org/jira/browse/SENTRY-2115
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> **Scenario-1:** When HDFS sync is disabled, and sentry is started for the 
> first time.
> **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
> less than last event-d processed by sentry
> **Scenario-3:** When HDFS sync is disabled, and first event-id in the 
> subsequent pull is not greater than the last event-id processed by sentry by 
> 1.
> **New Behavior:** Full snapshots need not be taken in all
> When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> 
> **Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
> and then HDFS sync is enabled and sentry service is restarted to get it to 
> effect.
> **New Behavior:** When Sentry detects out-of-sync situations, it should reset 
> SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
> HMS_NOTIFICATION_LOG from beginning.
> To handle scenario explained in Scenario-4, sentry should reset the mapping 
> information when ever HDFS sync is disabled. That way it can learn from 
> scratch when the feature is enabled back. There is no value is holding stale 
> data even when we know it will have issues when the feature is enabled back.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  2f2b98412e7dfdcc847ffe7975a70f452554e747 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  79030780c35e5bda432e3ec3f01328e627cb50a6 
> 
> 
> Diff: https://reviews.apache.org/r/65533/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-02-06 Thread kalyan kumar kalvagadda via Review Board 


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 138 (original), 138 (patched)
> > 
> >
> > 1) Please add space after //
> > 2) The comment should probably say something like: `These tests do not 
> > need to retry transactions, so limit transaction retries to half a second.

will update


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3424 (patched)
> > 
> >
> > Can we use something simpler to test this? What would be the simplest 
> > thing that we can use to cause exceptions?

I thought simpler way to simulate this failure was by inserting duplicate 
entries. That is what I have done. Let me know of you can think anything simpler


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3435 (patched)
> > 
> >
> > This can be just `new HashMap<>()`

will change.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3447 (patched)
> > 
> >
> > It would be better to simplify the message - e.g. 
> > `fail("expected JDODataStoreException")`

will change.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3449 (patched)
> > 
> >
> > Do you know that it is JDODataStoreException? WIll it be the same if 
> > the test is executed on non-Derby DB?

Performing a duplicate insert would result in JDODataStoreException(derived 
from javax.jdo.JDOCanRetryException) from datanucleus.

This exception is not specific to derby and should be seen even for other 
databases. Sentry store tries to catch the same exception in many API's.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3450 (patched)
> > 
> >
> > Unexpected failure isn't very useful - it is better to describe what 
> > went wrong - what you expected and what happened instead.

Will update.


> On Feb. 6, 2018, 1:32 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3453 (patched)
> > 
> >
> > millisecond is one word

Will update.


- kalyan kumar


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


On Feb. 5, 2018, 3:02 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 5, 2018, 3:02 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1904
> https://issues.apache.org/jira/browse/SENTRY-1904
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The TransactionManager uses exponential backoff strategy for transaction 
> retries. This may cause some transactions to be delayed by a very long time. 
> We should also have a constraint on the max time for a transaction so that we 
> do not retry for too long. 
> 
> New patch that is attached adds upper bounds on below
> 
> 1.Interval between the retry attempts which increases exponentially.
> 2.Total time a transaction could spend in retries.
> 3.Removed retry based on count.
>  
> 
> With out these limits we would not have a control on how long a transaction 
> could be be active.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
>  f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 
>   
>

Re: Review Request 65487: Bump com.codahale.metrics package to io.dropwizard.metrics version 3.2.2

2018-02-06 Thread Xinran Tinney 

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


Ship it!




Ship It!

- Xinran Tinney


On Feb. 2, 2018, 7:31 p.m., Liam Sargent wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65487/
> ---
> 
> (Updated Feb. 2, 2018, 7:31 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Steve 
> Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2136
> https://issues.apache.org/jira/browse/SENTRY-2136
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump com.codahale.metrics package to io.dropwizard.metrics version 3.2.2
> 
> 
> Diffs
> -
> 
>   pom.xml 6f9856e45b72ef9e0c43a222eddc8452b64f1a71 
>   sentry-provider/sentry-provider-db/pom.xml 
> 5733445af481fd83bb71189178647af234fe77a1 
>   sentry-tests/sentry-tests-solr/pom.xml 
> 5ef7a2b1de67a2f35510ad41c0150ad1bc957118 
> 
> 
> Diff: https://reviews.apache.org/r/65487/diff/1/
> 
> 
> Testing
> ---
> 
> mvn test - ALL PASS
> 
> 
> Thanks,
> 
> Liam Sargent
> 
>

Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.

2018-02-06 Thread kalyan kumar kalvagadda via Review Board 

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

Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.


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


Repository: sentry


Description
---

**Scenario-1:** When HDFS sync is disabled, and sentry is started for the first 
time.
**Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is 
less than last event-d processed by sentry
**Scenario-3:** When HDFS sync is disabled, and first event-id in the 
subsequent pull is not greater than the last event-id processed by sentry by 1.
**New Behavior:** Full snapshots need not be taken in all
When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.

**Scenario-4:** Initially HDFS sync was enabled and later disabled for while 
and then HDFS sync is enabled and sentry service is restarted to get it to 
effect.
**New Behavior:** When Sentry detects out-of-sync situations, it should reset 
SENTRY_HMS_NOTIFICATION_ID table and start processing the event in 
HMS_NOTIFICATION_LOG from beginning.
To handle scenario explained in Scenario-4, sentry should reset the mapping 
information when ever HDFS sync is disabled. That way it can learn from scratch 
when the feature is enabled back. There is no value is holding stale data even 
when we know it will have issues when the feature is enabled back.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 2f2b98412e7dfdcc847ffe7975a70f452554e747 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 79030780c35e5bda432e3ec3f01328e627cb50a6 


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


Testing
---

Made sure that all the tests passed.


Thanks,

kalyan kumar kalvagadda