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/#review196669
---




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


It would be nice to add comment explaining what's going on with the 
implementation.



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


This comment seems misplaced now.



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


What are you trying to do here? This is a non-trivial expression that 
requires explanation. Better yet to express it in a way that is obvious.



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


Why do you want to break here?

I guess what you are trying to do is 

if (startTimeStamp + maxTimeAllowed > currentTime) break;

Or you are trying to do something else?

If it is what you are trying to do it would be nice to pre-compute end time 
by adding startTimestamp and the limit and just compare curtrent time with the 
deadline time.



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


It may not be very important but I think incrementing retryCount before 
sleep is better.



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


WHy do you need both sleepTime and newSleepTime?



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


And what if it isn't?



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


At this point you know how much time you spent, so it would be nice to 
print it.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 138 (original), 138 (patched)


This comment is no longer correct



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3421 (patched)


I think it is better to have separate tests for duplicate notifications and 
for transactiuon timeout.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3426 (original), 3433 (patched)


Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3449 (original), 3456 (patched)


Why do you have extra spaces here?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3451 (original), 3458 (patched)


Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3453 (original), 3460 (patched)


Extra spaces



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 3481 (original), 3489 (patched)


WHy do you have spacing changes?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3495 (patched)


Even with the check it is possible that the time exceeds the allotted time 
- you need to give some margin of error or this will be a flaky test.

Re: How to run bin/run_sentry.sh

[Alexander Kolbasov]

Here is the one I was using - make sure to edit database settings. You also
need to have users to group mapping file - I use the following:

[users]
akolb = hive

Config attached.


On Thu, Feb 1, 2018 at 7:15 AM, Xinran Yu Tinney 
wrote:

> Hi, Sasha,
>The only sentry-site.xml I found is shown in the picture below, it
> seems still something is missing.
>
> Thanks!
>
> 2018-01-30 18:34 GMT-06:00 Alexander Kolbasov :
>
>> The way run_sentry works - it actually runs mvn exec:java which builds
>> code
>> in your local repo and creates jar files. If you run this as root, these
>> files will be owned by root which is not good, so please do not run this
>> via sudo to save you some trouble.
>>
>> A simple way to use it to run sentry:
>>
>> run_sentry --command  service -conffile /path/to/sentry-site.xml
>>
>> This should work as long as your normal things (like mvn install
>> -DskipTests) is working.
>>
>> Best,
>>
>> - Alex
>>
>> On Tue, Jan 30, 2018 at 1:41 PM, Xinran Yu Tinney > >
>> wrote:
>>
>> > Hi, Sasha, what do you mean change ownership of files?
>> > Also, Lina, I have run the command but it seems something removed the
>> > JAVA_HOME, please see the attachment.
>> >
>> > 2018-01-12 16:57 GMT-06:00 Alexander Kolbasov :
>> >
>> >> Running it via studio is not the best idea since it will change
>> ownership
>> >> of files in your repo. Sentry doesn't need root privileges.
>> >>
>> >> > On Jan 12, 2018, at 14:10, Na Li  wrote:
>> >> >
>> >> > one example
>> >> >
>> >> > to creating schema
>> >> > - command: sudo ./run_sentry.sh --command  schema-tool --conffile
>> >> > sentry-site.xml --dbType mysql --initSchema
>> >> >
>> >> > On Fri, Jan 12, 2018 at 3:18 PM, Xinran Yu Tinney <
>> >> yuxinran8...@gmail.com>
>> >> > wrote:
>> >> >
>> >> >> What are the arguments that can be used? I have attached a
>> screenshot,
>> >> >> using "service", but seems not working.
>> >> >>
>> >> >> 2018-01-12 14:56 GMT-06:00 Alexander Kolbasov :
>> >> >>
>> >> >>> It is easy. Your current directory should be within the got repo
>> and
>> >> the
>> >> >>> actual script can be anywhere. Just run it with whatever argument
>> you
>> >> want
>> >> >>> to pass to main. The script users maven to build and run sentry so
>> it
>> >> >>> doesn't depend on any class path.
>> >> >>>
>> >> >>> On Fri, Jan 12, 2018 at 12:30 Xinran Yu Tinney <
>> >> yuxinran8...@gmail.com>
>> >> >>> wrote:
>> >> >>>
>> >>  Hi,
>> >>    Does anyone know how to run bin/run_sentry.sh? Thanks!
>> >> 
>> >> 
>> >>  Xinran
>> >> 
>> >> >>>
>> >> >>
>> >> >>
>> >>
>> >
>> >
>>
>
>



  
sentry.service.server.rpc-address
localhost
  
  
sentry.service.client.server.rpc-address
localhost:8038
  
  
sentry.service.server.rpc-port
8038
  
  
sentry.service.security.mode
none
  
  
sentry.service.admin.group
hive,impala,hue,solr,kafka
  
  
sentry.service.allow.connect
hive,impala,hue,hdfs,solr,kafka, akolb
  
  
sentry.store.group.mapping
org.apache.sentry.provider.file.LocalGroupMappingService
  
  
sentry.store.group.mapping.resource
/Users/akolb/etc/users.ini
  
  
sentry.service.server.keytab
sentry.keytab
  
  
  
sentry.store.jdbc.url
jdbc:mysql://localhost/sentryserver
  
  
sentry.store.jdbc.driver
com.mysql.jdbc.Driver
  
  
sentry.store.jdbc.user
USER
  
  
sentry.store.jdbc.password
PASSWORD
  
  
sentry.service.processor.factories
org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessorFactory,org.apache.sentry.provider.db.generic.service.thrift.SentryGenericPolicyProcessorFactory,org.apache.sentry.hdfs.SentryHDFSServiceProcessorFactory
  
  
sentry.policy.store.plugins
org.apache.sentry.hdfs.SentryPlugin
  
  
sentry.hdfs.integration.path.prefixes
/user/hive/warehouse
  
  
  
sentry.service.web.enable
true
  
  
  
sentry.service.web.port
51000
  
  
sentry.web.admin.servlet.enabled
true
  
  
sentry.service.reporter
console
  
  
sentry.service.reporter.interval.sec
300
  
  
sentry.service.web.authentication.type
NONE
  
  
  
sentry.ha.zookeeper.quorum
127.0.0.1:2181
  
  
sentry.ha.standby.signal
USR2
  
  

Re: Issues with Codahale/Dropwizard metrics dependencies

[Alexander Kolbasov]

Hi Liam,

I don’t think there is a particular reason other then historical development. I 
think we should just align with whatever version is used by majority of other 
components or just use the latest version. I don’t see any reason to run with 
old version.

- Alex

> On Feb 1, 2018, at 12:55 PM, Liam Sargent  wrote:
> 
> Hello Sentry Folks,
> 
> I am working on patching some bugs in Sentry, and I am having a hard time
> getting my tests to run properly. It seems every time I run tests, I am
> running into dependency-related conflicts concerning a particular metrics
> package that supplies JVM statistics called "metrics", with several
> packages pointing to a version called com.codahale.metrics, and several
> pointing to io.dropwizard.metrics.
> 
> I believe these are both referencing different versions of the same
> packages, but a namespace change occurred somewhere along the 3.1 release
> that is confusing maven and thus, myself.
> 
> Does anyone have a definitive answer of which version of metrics should be
> on the classpath for test execution and development? Many Hadoop
> dependencies point at 3.2.2 (dropwizard), but our main pom.xml points at
> 3.0.2 (codahale)
> 
> 
> Is there any reason we are using this version? Could we safely bump the
> version to dropwizard 3.2.2?
> 
> Cheers,
> Liam Sargent

Issues with Codahale/Dropwizard metrics dependencies

[Liam Sargent]

Hello Sentry Folks,

I am working on patching some bugs in Sentry, and I am having a hard time
getting my tests to run properly. It seems every time I run tests, I am
running into dependency-related conflicts concerning a particular metrics
package that supplies JVM statistics called "metrics", with several
packages pointing to a version called com.codahale.metrics, and several
pointing to io.dropwizard.metrics.

I believe these are both referencing different versions of the same
packages, but a namespace change occurred somewhere along the 3.1 release
that is confusing maven and thus, myself.

Does anyone have a definitive answer of which version of metrics should be
on the classpath for test execution and development? Many Hadoop
dependencies point at 3.2.2 (dropwizard), but our main pom.xml points at
3.0.2 (codahale)


Is there any reason we are using this version? Could we safely bump the
version to dropwizard 3.2.2?

Cheers,
Liam Sargent

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

[Vadim Spector via Review Board]

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


Ship it!




Ship It!

- Vadim Spector


On Jan. 30, 2018, 1:08 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64955/
> ---
> 
> (Updated Jan. 30, 2018, 1:08 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio 
> Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-2109
> https://issues.apache.org/jira/browse/SENTRY-2109
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch does couple of things
> 1. Avoid triggering full snapshots when gaps are observed while fetching new 
> notifications. While fetching new notifications HMSFollower would would fetch 
> notifications with last event-id as well. When it gets the notifications and 
> if it doesn't get the notifications with event-id, full snpshot is triggered.
> 2. Functinalty to address gaps and out-of-sequence notificaitons by 
> re-fetching addtional notifications that were already fetched. This solution 
> is not fool proof. It does a best effort to reduse the chance of loosing 
> notifications by re-fetching the notifications.This approach will introduce 
> an over head of fetching addtional notifications that were already fetched. 
> Overhead of DB look-up is addressed by using a cache. This reduces additional 
> DB lookups needed to check if the notification was already processed.
> 2. Added looging to report duplicate events and out of order events for debug 
> purpose.
> 3. Added new e2e tests to verfy this behavior.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryOutOfSyncException.java
>  PRE-CREATION 
>   
> 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/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
>  93cc34f34c044dceb11d27e41ecbd1a14f64bed9 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 
>   
> 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/TestHMSFollower.java
>  79030780c35e5bda432e3ec3f01328e627cb50a6 
>   
> 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 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java
>  83a1becd46ac2d69c7d6dd05ed6253d1cdd8800d 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcherCache.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotCreation.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotCreationWithShorterHMSEventTtl.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotWithLongerHMSFollowerLongerInterval.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4cd00e6672730773c74b9840247d1f4d5f7bdfe4 
> 
> 
> Diff: https://reviews.apache.org/r/64955/diff/8/
> 
> 
> Testing
> ---
> 
> Made sure that tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>