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

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 221 (original), 227 (patched)


Try execurting transaction. If it fails due to SentryUserException, 
propagate the exception immediately. Otherwise retry transaction multiple times 
trying not to exceed the total time limit.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 224 (original), 240 (patched)


I sould suggest something along the following lines:

`
  private class ExponentialBackoff {

@SuppressWarnings("squid:S00112")
 T execute(Callable arg) throws Exception {
  Exception ex = null;
  int retryNum = 0;
  // Maximumm total sleep time
  long sleepTime = maxRetryDurationMills / 32;
  // Maximum sleep time between retries
  final long maxSleep = maxRetryDurationMills / 4;

  final long deadline = System.currentTimeMillis() + 
maxRetryDurationMills;

  do {
try {
  return arg.call();
} catch (SentryUserException e) {
  // throw the sentry exception without retry
  LOGGER.warn("Transaction manager encountered non-retriable 
exception", e);
  throw e;
} catch (Exception e) {
  ex = e;
  LOGGER.warn("Transaction execution encountered exception", e);
  long remainingTime = deadline - System.currentTimeMillis();
  if (remainingTime <= 0) {
break;
  }

  if (sleepTime < maxSleep) {
// Introduce some randomness in the backoff time.
int fuzz = random.nextInt((int) sleepTime / 2);
sleepTime *= 3;
sleepTime /= 2;
sleepTime += fuzz;
  }
  sleepTime = Math.min(sleepTime, Math.min(maxSleep, 
remainingTime));
  LOGGER.warn("Retrying transaction {} times, sleeping for {} ms", 
++retryNum, sleepTime);
  retryCount.inc();
  Thread.sleep(sleepTime);
}
  } while (true);
  assert (ex != null);
  String message = "Retried for " + maxRetryDurationMills + " 
milliseconds. Giving up "
  + ex.getMessage();
  LOGGER.error(message, ex);
  throw new Exception(message, ex);
}
  }
`


- Alexander Kolbasov


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 7, 2018, 6:04 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 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Sergio Pena via Review Board]

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



I left a few comments, but I need to go early so I would leave some comments 
later.


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 240 (original), 258 (patched)


Is there a way to get rid of the isLastRetry? Can we avoid it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 243 (original), 264 (patched)


Should we store this time in a variable instead of calling 3 times in this 
if() statement?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 273 (patched)


Why is this warn() moved after the sleep instead of the original one which 
was before the sleep? is there a benefit?


- Sergio Pena


On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 7, 2018, 6:04 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 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  7e02874b4be6a7109108809b1f404fe971b7b8e2 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  91c90f9d302f4feb3a8b3d06541f43541c87bf0f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Sergio Pena via Review Board]

> On Feb. 8, 2018, 4:13 p.m., Sergio Pena wrote:
> > Ship It!

Thanks Liam for the patch. I checked that there were no more codehale jars 
included in the classpath (+1), and that other components, such as Hive, uses 
the new dropwizard library instead of codehale. So, it makes sense to make this 
change on Sentry as well.


- Sergio


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


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

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

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


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

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

[Sergio Pena via Review Board]

> On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Line 205 (original), 212-213 (patched)
> > 
> >
> > Isn't the new method name more confusing than before? 
> > isFullSnapshotRequired() means Sentry requires a full HMS snapshot while 
> > isReSyncRequired() sounds like Sentry requires to sync with HDFS, but the 
> > hdfsSyncEnable after this contradicts the name, isn't it?
> 
> kalyan kumar kalvagadda wrote:
> synching in HMSFollower meaning synchornizing with HMS. Defination of 
> being synchronized with HMS differs when HDFS sync feature is enabled OR 
> disable.

I think we should keep the isFullSnapshotRequired() name as the meaning makes 
more sense for what it does 'see if a full HMS snapshot is required'. 
isReSyncRequired() could conflict with HDFS sync meaning which they're 
different.


> On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Lines 217-220 (patched)
> > 
> >
> > This needs a better explanation. I'm trying to understand the reason of 
> > these lines. A re-sync with HDFS needed? But HDFS is disabled, right?
> 
> kalyan kumar kalvagadda wrote:
> re-synching in HMSFollower meaning synchornizing with HMS. 
> Let's hdfs sync is disabled and sentry detects that it is out of sync, it 
> should set the last processed eventid as 0 and process all the notification 
> event in HMS NOTIFICATION_LOG table.

I see that there is an if (hdfsSyncEnabled) inside the createFullSnapshot() 
method. Can we put this block inside the createFullSnapshot() method instead?


> On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Line 254 (original), 274 (patched)
> > 
> >
> > Why do we need a method change? Is this resync meant for hdfs or hms 
> > full snapshot?
> 
> kalyan kumar kalvagadda wrote:
> Idea of this patch is to avaoid getting full snapshot when hdfs sync is 
> not enabled.
> 
> method isFullSnapshotRequired, tells us if sentry is out-of-sync with 
> HMS. 
> 
> isFullSnapshotRequired is not correct. Please suggest if 
> "isReSyncRequired" is not reasonable.

isFullSnapshopt() is more reasonable. isReSyncRequired() conflicts with 
areNotificationOutOfSync() and Hdfs Sync.


- Sergio


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


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
>