Re: Review Request 63086: SENTRY-1993: StringIndexOutOfBoundsException in HMSPathsDumper.java

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 17, 2017, 7:12 p.m., Misha Dmitriev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63086/
> ---
> 
> (Updated Oct. 17, 2017, 7:12 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Vadim Spector.
> 
> 
> Bugs: SENTRY-1993
> https://issues.apache.org/jira/browse/SENTRY-1993
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1993: StringIndexOutOfBoundsException in HMSPathsDumper.java
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  1267093dbbdeb291ec01ecdc87253a90e8ab98ac 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java
>  6a4e32f6ad180bc1c2ecb6d0be7cdae6c586505d 
> 
> 
> Diff: https://reviews.apache.org/r/63086/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Misha Dmitriev
> 
>

Re: Do Sentry sync-up meetings exist?

[Alexander Kolbasov]

How do we log in there? It doesn't recognize my slack user/password or
apache user/password


On Thu, Oct 19, 2017 at 11:45 AM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> I created a #sentry channel on the-asf.slack.com. This URL has other
> Apache channels, so it would be good to move our current apache-sentry
> slack to this official apache slack.
>
> On Mon, Oct 16, 2017 at 3:14 AM, Colm O hEigeartaigh <cohei...@apache.org>
> wrote:
>
>> I believe you can sign up here: https://the-asf.slack.com/
>>
>> Colm.
>>
>> On Thu, Oct 5, 2017 at 8:51 PM, Alexander Kolbasov <ak...@cloudera.com>
>> wrote:
>>
>> > Yes, moving to the official ASF slack team would make a lot of sense.
>> What
>> > should we do to move there?
>> >
>> >
>> > On Thu, Oct 5, 2017 at 1:48 AM, Colm O hEigeartaigh <
>> cohei...@apache.org>
>> > wrote:
>> >
>> >> I joined the slack team. There's an official "ASF" slack team with
>> rooms
>> >> for various projects, it might be better to move it there long-term.
>> >>
>> >> Colm.
>> >>
>> >> On Wed, Oct 4, 2017 at 11:22 PM, Alexander Kolbasov <
>> ak...@cloudera.com>
>> >> wrote:
>> >>
>> >> > I don’t want to make it too official yet - this is experimental thing
>> >> > using my private slack account - and so far it isn’t very active.
>> Let’s
>> >> > test-drive it for a while and then see where we can put it.
>> >> >
>> >> > - Alex
>> >> >
>> >> > > On Oct 4, 2017, at 3:21 PM, Sergio Pena <sergio.p...@cloudera.com>
>> >> > wrote:
>> >> > >
>> >> > > Sasha, should we add this information about Slack rooms here and
>> how
>> >> to
>> >> > > request an invitation?
>> >> > > http://sentry.apache.org/community/get_involved.html
>> >> > >
>> >> > > On Wed, Oct 4, 2017 at 5:19 PM, Alexander Kolbasov <
>> >> ak...@cloudera.com>
>> >> > > wrote:
>> >> > >
>> >> > >> It is https://apache-sentry.slack.com <
>> https://apache-sentry.slack.c
>> >> om/
>> >> > >
>> >> > >> - I created room in my private slack account - sent Colm an
>> invite.
>> >> > >>
>> >> > >>
>> >> > >>> On Oct 4, 2017, at 1:36 AM, Colm O hEigeartaigh <
>> >> cohei...@apache.org>
>> >> > >> wrote:
>> >> > >>>
>> >> > >>> Where is the Slack room? I don't see it in the ASF team.
>> >> > >>>
>> >> > >>> Colm.
>> >> > >>>
>> >> > >>> On Wed, Oct 4, 2017 at 1:15 AM, Alexander Kolbasov <
>> >> ak...@cloudera.com
>> >> > >
>> >> > >>> wrote:
>> >> > >>>
>> >> > >>>> Sravya used to have meetings over hangout, it was attended by
>> >> Colin Ma
>> >> > >> and
>> >> > >>>> some other folks. It is definitely a good idea if there is an
>> >> > interest.
>> >> > >>>>
>> >> > >>>> Also a reminder that we have a slack room for discussions.
>> >> > >>>>
>> >> > >>>> On Tue, Oct 3, 2017 at 2:50 PM, Sergio Pena <
>> >> sergio.p...@cloudera.com
>> >> > >
>> >> > >>>> wrote:
>> >> > >>>>
>> >> > >>>>> Hi All,
>> >> > >>>>>
>> >> > >>>>> Did we have any kind of sync-up meetings among the Sentry
>> >> community
>> >> > in
>> >> > >>>> the
>> >> > >>>>> past?
>> >> > >>>>>
>> >> > >>>>> I've been in other Apache communities where they have these
>> google
>> >> > chat
>> >> > >>>>> sync-up meetings every 1 or 2 months to talk about what they'd
>> >> like
>> >> > to
>> >> > >> do
>> >> > >>>>> next in their products. Do you guys think this should be a
>> >> reasonable
>> >> > >>>> thing
>> >> > >>>>> to do?
>> >> > >>>>>
>> >> > >>>>> - Sergio
>> >> > >>>>>
>> >> > >>>>
>> >> > >>>
>> >> > >>>
>> >> > >>>
>> >> > >>> --
>> >> > >>> Colm O hEigeartaigh
>> >> > >>>
>> >> > >>> Talend Community Coder
>> >> > >>> http://coders.talend.com
>> >> > >>
>> >> > >>
>> >> >
>> >> >
>> >>
>> >>
>> >> --
>> >> Colm O hEigeartaigh
>> >>
>> >> Talend Community Coder
>> >> http://coders.talend.com
>> >>
>> >
>> >
>>
>>
>> --
>> Colm O hEigeartaigh
>>
>> Talend Community Coder
>> http://coders.talend.com
>>
>
>

Re: Shading of conflicting dependencies

[Alexander Kolbasov]

Agreed, this would be a very useful thing to do. I remember spending a lot
of time trying to make Sentry work with DataNucleus 4 - the problem was
that e2e tests combine Sentry with Hive in the same JVM and this created a
conflict on the DataNucleus libraries and test failures.

Looking at the HBase proposal it seems to require some (small) code changes
for package names - or I am misreading it? Do you plan to shade all 3-rd
party packages or just some? I would consider at least
Guava/ZooKeepeer/DataNucleus/Jetty.

- Alex

On Wed, Nov 15, 2017 at 8:24 AM, Brian Towles  wrote:

> Howdy all,
>
> An issue that keeps coming up seems to be the conflict of dependency
> versions between Sentry and the components it is plugging into. A current
> example of this impact is Google Guava with hive2 using v14 and Impala
> using v11 while Sentry needs to have at least v14 in order to fix some bugs
> in the BoneCP library.  These sort of conflicts come in whenever we are
> embedding a Sentry plugin or using a Sentry library into another project.
>
> I would like to propose a mechanism for offsetting some of these issues
> using something similar to the third party shading that HBase uses for
> common problems (such as Guava)
> (https://github.com/apache/hbase-thirdparty with
> documentation about it http://hbase.apache.org/book.html#thirdparty) and
> is
> used by HBase and Hadoop for packaging of their downstream artifacts (
> https://github.com/apache/hbase/blob/master/hbase-shaded/ and
> https://github.com/apache/hadoop/tree/trunk/hadoop-
> client-modules/hadoop-client-api/
> )
>
> The main benefit of this is that it would allow Sentry to be used as
> libraries and plugins with all of the dependencies needed for Sentry to be
> abstracted away from components implementing it. Sentry could rev versions
> of libraries easier and not have collisions of library versions needed by
> the implementing component.  As well this would potentially make Sentry
> downstream usage more stable since it would be used and tested against a
> static set of dependencies and not using libraries based on what the
> implementing component has.
>
> On the downside, it would make the on disk size of the Sentry plugins and
> libraries for downstream larger. As well, the number of classes loaded into
> memory would be larger since there would be potential duplication of actual
> class implementations or multiple versions of a class with different
> package names in memory.  But this seems to be a common practice and the
> lack of library version collisions and stability make up for these
> downsides.
>
> The third party shading works by using the Maven shade plugin to do package
> name shifting of the third party library (Guava, BoneCP) to a sentry
> specific package using the version on the third party library needed for
> Sentry.
>
> E.G. *com.google.common* packages could be shifted to
> *org.apache.sentry.shaded.com.google.common*.
>
> Since the Maven Shade plugin can actually change the byte-codes of the
> libraries being shaded, we can do this even for dependencies that have
> shared sub-dependencies .  BoneCP being the main example since it uses
> Guava internal to itself.  We could shade the BoneCP into the same shared
> sentry third party dependency jar and since the bytecode level
> manipulation.
>
> What this means on the development side is that we would need to reference
> imports from the shaded third party
>
> E.G. *import com.google.common.collect.Maps* becomes *import
> org.apache.sentry.shaded.com.google.common.collect.Maps*.
>
> Ive been looking at this in the context of
> https://issues.apache.org/jira/browse/SENTRY-2044, but I feel this should
> potentially be something that is more of an overall Sentry standard
> practice and larger scale implementation.
>
> -=Brian
>
>
> --
> *Brian Towles* | Software Engineer
> t. (512) 415- <00>8105 e. btow...@cloudera.com 
> cloudera.com 
>
> [image: Cloudera] 
>
> [image: Cloudera on Twitter]  [image:
> Cloudera on Facebook]  [image: Cloudera
> on LinkedIn] 
> --
>

Re: Shading of conflicting dependencies

[Alexander Kolbasov]

Brian, There are cases where shading is useful for server as well - mostly
for e2e tests which run server, HDFS, Hive all in the same JVM.

On Wed, Nov 15, 2017 at 12:47 PM, Brian Towles <btow...@cloudera.com> wrote:

> So there would be two different levels of shading.
>
> The first would be the known issues shared repackaged dependencies like
> Guava.  This would only be for libraries that we absolutely know will cause
> collisions with implementing services and implementing third party
> libraries that need to have the references to the trouble causing libraries
> shifted.  These trouble causing libraries would be the only ones where hard
> coded import path changes would need to take place for.
>
> For example a sentry-thridparty/sentry-shaded-miscellaneous module would
> shared google guava shifting the package to
> org.apache.sentry.thirdparty.com.google.common. As well as it would shade
> the BoneCP jar so that it picks up the shift of the Guava package but not
> shifting the BoneCP packages unless we encounter and a known conflict.
>
> The second level os shading would be for the plugins distribution and would
> shade and package shift only the dependencies needed for that plugin.  This
> would not need to have any import statement changes as the maven shade
> plugin would change the bytecode package references on packaging.  As to
> Kalyan's question, the run time memory used for class loading would only be
> for those items used by the plugin, which is really not much anyways
> comparatively. The class loader doesn't load up a class until its needed.
> Till that point it is just a reference to a file in a jar.  The list of
> files/classes in jars is not a heavy memory load.
>
> The main server implementation would not need to have the second level
> shading done to it as it runs stand alone and is unaffected by the
> collisions issues.
>
> @Alexander For your questions, there is a small amount of refactoring of
> import statements that need to take place for the first level of shading
> but there are no logic changes that take place.
>
> @Stephen Yeah I can see BouncyCastle being more of an issue but it does not
> appear in any of the Sentry dependency chains right now.
>
> -=Brian
>
> On Wed, Nov 15, 2017 at 1:30 PM Stephen Moist <mo...@cloudera.com> wrote:
>
> > +1
> >
> > Agree with Kalyan’s concern.  To me it seems simpler to shade all jars
> > rather than known issue jars.  One thing though is what if there are bug
> > fixes in our jars that are not included in other components, it may cause
> > subtle error that are hard to track.  I’m thinking along the lines of the
> > KeyValue class change in behavior.  So I propose limited the scope of
> > shaded jars to known issues.
> >
> > However one thing to note is that some jars do not like to be shaded, as
> > an example BouncyCastle has a check to see if the jar has been modified
> and
> > will refuse to be used in a shaded jar.
> >
> > > On Nov 15, 2017, at 1:07 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> > >
> > > +1
> > >
> > > It's good approach but I have a question/concern.
> > > Is the proposal to shade is for some specific jars OR to shade all the
> > > third party jars?
> > >
> > > If proposal is shade all the third-party jars then if would impact the
> > run
> > > time memory usage as all the classes from the third-party jars would be
> > > loaded regardless..
> > >
> > > -Kalyan
> > >
> > > On Wed, Nov 15, 2017 at 12:04 PM, Sergio Pena <
> sergio.p...@cloudera.com>
> > > wrote:
> > >
> > >> +1
> > >>
> > >> I like the idea. It's hard to upgrade our libraries to newer ones when
> > >> other components break due to Sentry being a plugin. I was once
> thought
> > of
> > >> using different jars versions per plugin (i.e. guava11 on hdfs,
> guava14
> > on
> > >> hive and the server, etc), but that is too much to do and not good. I
> > like
> > >> the hbase idea better so we can use the latest libraries that we want
> > >> instead of depending on other components.
> > >>
> > >> - Sergio
> > >>
> > >>
> > >>
> > >> On Wed, Nov 15, 2017 at 11:53 AM, Alexander Kolbasov <
> > ak...@cloudera.com>
> > >> wrote:
> > >>
> > >>> Agreed, this would be a very useful thing to do. I remember spending
> a
> > >> lot
> > >>> of time trying to make Sentry work with DataNucleus 4 - 

Re: Contributorship

[Alexander Kolbasov]

Added you as a contributor in JIRA

On Tue, Nov 14, 2017 at 2:52 PM, Liam Sargent 
wrote:

> Hello all,
>
> I would like to request the contributor role on issues.apache.org for the
> SENTRY project. My apache username is "liamsargent"
>
> Thank you in advance,
> Liam Sargent
>

Re: Becoming contributor

[Alexander Kolbasov]

I added you to the contributors group - please let me know if this isnt
working for you.

On Tue, Nov 14, 2017 at 11:46 AM, Xinran Yu Tinney 
wrote:

> Hi,
>I would like to become Sentry contributor and my Apache Jira username is
> xyu2017.
>
> Thanks!
>
> Xinran Tinney
>

2.0 release and SENTRY-1812

[Alexander Kolbasov]

Colm, do you plan to include SENTRY-1812 in 2.0 release?

Re: [VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

Kalyan,

Thank you for posting Release 2.0 candidate. What kind if testing would you
recommend for us to perform to vote Yes/No? Can you post some
recommendations, please?

- Alex

On Tue, Nov 28, 2017 at 4:38 PM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> This is the release of Apache Sentry, version 2.0.0.
>
> It fixes the following issues:
> *https://issues.apache.org/jira/projects/SENTRY/versions/12341081
> *
>
> Source and bin files :
> *http://home.apache.org/~kalyan/apache-sentry-2.0.0-src-rc-1/
> *
>
> Maven artifacts are available
> here:https://repository.apache.org/content/repositories/orgapachesentry-
> 1005/
>
>
> Tag to be voted on
> *https://git-wip-us.apache.org/repos/asf/sentry/?p=
> sentry.git;a=tag;h=refs/tags/release-2.0.0
>  sentry.git;a=tag;h=refs/tags/release-2.0.0>*
>
> Sentry's KEYS containing the PGP key we used to sign the release:
> http://www.apache.org/dist/sentry/KEYS
>
>  we are voting on the source:tag=release-2.0.0, SHA=
> 18fe7c596fa1ffad3e656a42d534ac190876b642
>  (You can get the hash of the tag by doing "git rev-list release-1.8.0 |
> head -n 1" )
>
> Vote will be open for 72 hours.
>
> [ ] +1 approve
> [ ] +0 no opinion
> [ ] -1 disapprove (and reason why)
>
> -Kalyan
>

Re: [VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

sergio.p...@cloudera.com>
wrote:

> I don't have that issue.
>
> I downloaded the 2.0 tar.gz that is already signed and install it on an
> Ubuntu 16.04 machine with Hive 2.3.2. It has been running in a single node
> mode. I could check HMS notifications being pulled and updated, and
> checking the permissions.
>
> On Wed, Nov 29, 2017 at 12:20 PM, Kalyan Kumar Kalvagadda <
> kkal...@cloudera.com> wrote:
>
> > Sergio,
> >
> > I know that you tried to start sentry standalone, Did you see this issue?
> >
> >
> > -Kalyan
> >
> > On Wed, Nov 29, 2017 at 12:16 PM, Alexander Kolbasov <ak...@cloudera.com
> >
> > wrote:
> >
> > > I tried to start Sentry server on Mac and got a weird error . I am
> > running
> > > from the dist directory that I just built with mvn clean install on the
> > > 2.0.0 branch.
> > >
> > > 17/11/29 10:13:25 ERROR thrift.SentryService: Error starting server
> > > java.lang.SecurityException: class "javax.servlet.DispatcherType"'s
> > signer
> > > information does not match signer information of other classes in the
> > same
> > > package
> > > at java.lang.ClassLoader.checkCerts(ClassLoader.java:898)
> > > at java.lang.ClassLoader.preDefineClass(ClassLoader.java:668)
> > > at java.lang.ClassLoader.defineClass(ClassLoader.java:761)
> > > at
> > > java.security.SecureClassLoader.defineClass(
> SecureClassLoader.java:142)
> > > at java.net.URLClassLoader.defineClass(URLClassLoader.
> java:467)
> > > at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
> > > at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
> > > at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
> > > at java.security.AccessController.doPrivileged(Native Method)
> > > at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
> > > at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
> > > at sun.misc.Launcher$AppClassLoader.loadClass(
> Launcher.java:331)
> > > at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
> > > at
> > > org.apache.sentry.service.thrift.SentryService.startSentryWebServer(
> > > SentryService.java:422)
> > > at
> > > org.apache.sentry.service.thrift.SentryService.
> > > runServer(SentryService.java:268)
> > > at
> > > org.apache.sentry.service.thrift.SentryService.call(
> > > SentryService.java:198)
> > > at
> > > org.apache.sentry.service.thrift.SentryService.call(
> > SentryService.java:76)
> > > at java.util.concurrent.FutureTask.run(FutureTask.java:266)
> > > at
> > > java.util.concurrent.ThreadPoolExecutor.runWorker(
> > > ThreadPoolExecutor.java:1142)
> > > at
> > > java.util.concurrent.ThreadPoolExecutor$Worker.run(
> > > ThreadPoolExecutor.java:617)
> > > at java.lang.Thread.run(Thread.java:745)
> > > Exception in thread "main" java.util.concurrent.ExecutionException:
> > > java.lang.Exception: Error starting server
> > > at java.util.concurrent.FutureTask.report(FutureTask.java:122)
> > > at java.util.concurrent.FutureTask.get(FutureTask.java:192)
> > > at
> > > org.apache.sentry.service.thrift.SentryService$
> > > CommandImpl.run(SentryService.java:591)
> > > at org.apache.sentry.SentryMain.main(SentryMain.java:122)
> > > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> > > at
> > > sun.reflect.NativeMethodAccessorImpl.invoke(
> > NativeMethodAccessorImpl.java:
> > > 62)
> > > at
> > > sun.reflect.DelegatingMethodAccessorImpl.invoke(
> > > DelegatingMethodAccessorImpl.java:43)
> > > at java.lang.reflect.Method.invoke(Method.java:498)
> > > at org.apache.hadoop.util.RunJar.run(RunJar.java:234)
> > > at org.apache.hadoop.util.RunJar.main(RunJar.java:148)
> > > Caused by: java.lang.Exception: Error starting server
> > > at
> > > org.apache.sentry.service.thrift.SentryService.call(
> > > SentryService.java:202)
> > > at
> > > org.apache.sentry.service.thrift.SentryService.call(
> > SentryService.java:76)
> > > at java.util.concurrent.FutureTask.run(FutureTask.java:266)
> > > at
> > > java.util.con

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2840 (patched)
<https://reviews.apache.org/r/63881/#comment270164>

Note that you are loggint this before transaction committed, (or even 
before you write actual snapshot) so it may be possible that you'll see the log 
that snapshot is created but it would not be actually created due to some issue 
with transaction commit.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2912 (original), 2913 (patched)
<https://reviews.apache.org/r/63881/#comment270165>

It would be also useful to log currentSnapshotID. Also, this is probably 
warning, not error. Do you want to log the full authzObject here or jost some 
relevant info from it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 424 (patched)
<https://reviews.apache.org/r/63881/#comment270166>

Message can be quite big - do we really want to log it?


- Alexander Kolbasov


On Nov. 29, 2017, 5:28 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 29, 2017, 5:28 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/4/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: [VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

Given that the licensing exercise isn't complete, I suggest revoking the
vote on the release until it is complete.

- Alex

On Thu, Nov 30, 2017 at 7:11 AM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> here are my next steps towards being compliant to apache licensing
>
> 1. Stop distributing jar which are licensed under GPL and LGPL only as they
> are not allowed to be distributed. (Please refer
> http://www.apache.org/legal/resolved.html#category-a)
> 2. See if we can stop distributing jars which are not licensed by Apache.
> 2. Document the rest of them in the LICENSE.txt following the guidelines
> mentioned in (https://www.apache.org/dev/licensing-howto.html)
>
> -Kalyan
>
> On Thu, Nov 30, 2017 at 4:40 AM, Colm O hEigeartaigh <cohei...@apache.org>
> wrote:
>
> > Hi Kalyan,
> >
> > You need to read the following page, which lays it all out:
> >
> > https://www.apache.org/dev/licensing-howto.html
> >
> > In particular, there are two things you need to do:
> >
> > a) Go through the list of dependencies and isolate the non-Apache
> licensed
> > jars. Include that license in a "licenses" directory and point to it in
> our
> > LICENSE.txt (note - it must be a permissive license, for example we can't
> > bundle LGPL jars that are not dual licensed):
> >
> > From the howto:
> >
> > "In LICENSE, add a pointer <http://s.apache.org/Hqj> to the dependency's
> > license within the distribution and a short note summarizing its
> licensing:
> >
> > This product bundles SuperWidget 1.2.3, which is available under a
> > "3-clause BSD" license.  For details, see deps/superwidget/.
> > "
> >
> > b) For all dependencies INCLUDING Apache licensed dependencies, you need
> > to go to the source for each dependency and see if it has a NOTICE file
> > with copyright notices that are non-Apache. If so they need to be
> included
> > in our NOTICE file.
> >
> > From the howto:
> >
> > "If the dependency supplies a NOTICE file, its contents must be analyzed
> > and the relevant portions bubbled up into the top-level NOTICE file."
> >
> > Colm.
> >
> >
> > On Wed, Nov 29, 2017 at 8:59 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> >> I have gathered all license information for all the dependencies using
> >> license-maven-plugin.
> >> Here is the file attached.
> >>
> >> Is there a particular format I should be using to update the license
> file
> >> in the repo?
> >>
> >> -Kalyan
> >>
> >> On Wed, Nov 29, 2017 at 1:52 PM, Colm O hEigeartaigh <
> cohei...@apache.org
> >> > wrote:
> >>
> >>> Hi Kalyan,
> >>>
> >>> Anything Apache related you can immediately discount - so Hadoop, Hive,
> >>> HBase, Solr, Zookeeper, etc. I would suggest just deleting those jars
> >>> from
> >>> the list and googling the remaining jars to see what the license is.
> >>>
> >>> Colm.
> >>>
> >>> On Wed, Nov 29, 2017 at 6:55 PM, Kalyan Kumar Kalvagadda <
> >>> kkal...@cloudera.com> wrote:
> >>>
> >>> > I'm trying to gather the license information for the dependencies.
> I'm
> >>> > trying to use Apache Maven Project Info Reports Plugin
> >>> > <https://maven.apache.org/plugins/maven-project-info-reports-plugin/
> >
> >>> .
> >>> >
> >>> > Does anyone has a better suggestion?
> >>> >
> >>> > -Kalyan
> >>> >
> >>> > On Wed, Nov 29, 2017 at 12:32 PM, Sergio Pena <
> >>> sergio.p...@cloudera.com>
> >>> > wrote:
> >>> >
> >>> > > I don't have that issue.
> >>> > >
> >>> > > I downloaded the 2.0 tar.gz that is already signed and install it
> on
> >>> an
> >>> > > Ubuntu 16.04 machine with Hive 2.3.2. It has been running in a
> single
> >>> > node
> >>> > > mode. I could check HMS notifications being pulled and updated, and
> >>> > > checking the permissions.
> >>> > >
> >>> > > On Wed, Nov 29, 2017 at 12:20 PM, Kalyan Kumar Kalvagadda <
> >>> > > kkal...@cloudera.com> wrote:
> >>> > >
> >>> > > > Sergio,
> >>> > > >
> >>> > > > I know that 

Re: [VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

When I enable Web UI I have this exception that I posted here. It goes away
when web UI is disabled. The exception is the same whether I run from the
target directory in a branch or using downloaded bits.

- Alex

On Wed, Nov 29, 2017 at 7:10 PM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> There is one more blocker for Sentry 2.0 that needs to be committed, can
> someone help me review this patch?
> https://issues.apache.org/jira/browse/SENTRY-2079
>
> I am able to configure Sentry 2.0 without other issues now. I can also
> enable the Sentry Web UI.
> What problems do you guys have with the Web UI?
>
> - Sergio
>
> On Wed, Nov 29, 2017 at 6:23 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > When I disabled Web UI the problem went away, so it is directly related
> to
> > Web UI.
> >
> > On Wed, Nov 29, 2017 at 3:12 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > Sasha,
> > >
> > > I have started sentry enabling web server as well. I do not see an
> > > exception and the webserver is successfully listening on the configured
> > > port.
> > >
> > > -Kalyan
> > >
> > > On Wed, Nov 29, 2017 at 2:59 PM, Kalyan Kumar Kalvagadda <
> > > kkal...@cloudera.com> wrote:
> > >
> > > > I have gathered all license information for all the dependencies
> using
> > > > license-maven-plugin.
> > > > Here is the file attached.
> > > >
> > > > Is there a particular format I should be using to update the license
> > file
> > > > in the repo?
> > > >
> > > > -Kalyan
> > > >
> > > > On Wed, Nov 29, 2017 at 1:52 PM, Colm O hEigeartaigh <
> > > cohei...@apache.org>
> > > > wrote:
> > > >
> > > >> Hi Kalyan,
> > > >>
> > > >> Anything Apache related you can immediately discount - so Hadoop,
> > Hive,
> > > >> HBase, Solr, Zookeeper, etc. I would suggest just deleting those
> jars
> > > from
> > > >> the list and googling the remaining jars to see what the license is.
> > > >>
> > > >> Colm.
> > > >>
> > > >> On Wed, Nov 29, 2017 at 6:55 PM, Kalyan Kumar Kalvagadda <
> > > >> kkal...@cloudera.com> wrote:
> > > >>
> > > >> > I'm trying to gather the license information for the dependencies.
> > I'm
> > > >> > trying to use Apache Maven Project Info Reports Plugin
> > > >> > <https://maven.apache.org/plugins/maven-project-info-
> > reports-plugin/>
> > > .
> > > >> >
> > > >> > Does anyone has a better suggestion?
> > > >> >
> > > >> > -Kalyan
> > > >> >
> > > >> > On Wed, Nov 29, 2017 at 12:32 PM, Sergio Pena <
> > > sergio.p...@cloudera.com
> > > >> >
> > > >> > wrote:
> > > >> >
> > > >> > > I don't have that issue.
> > > >> > >
> > > >> > > I downloaded the 2.0 tar.gz that is already signed and install
> it
> > on
> > > >> an
> > > >> > > Ubuntu 16.04 machine with Hive 2.3.2. It has been running in a
> > > single
> > > >> > node
> > > >> > > mode. I could check HMS notifications being pulled and updated,
> > and
> > > >> > > checking the permissions.
> > > >> > >
> > > >> > > On Wed, Nov 29, 2017 at 12:20 PM, Kalyan Kumar Kalvagadda <
> > > >> > > kkal...@cloudera.com> wrote:
> > > >> > >
> > > >> > > > Sergio,
> > > >> > > >
> > > >> > > > I know that you tried to start sentry standalone, Did you see
> > this
> > > >> > issue?
> > > >> > > >
> > > >> > > >
> > > >> > > > -Kalyan
> > > >> > > >
> > > >> > > > On Wed, Nov 29, 2017 at 12:16 PM, Alexander Kolbasov <
> > > >> > ak...@cloudera.com
> > > >> > > >
> > > >> > > > wrote:
> > > >> > > >
> > > >> > > > > I tried to start Sentry server on Mac and got a weird error
> .
> > I
> > > am
> > > >

Re: Review Request 64230: SENTRY-2082: Exclude javax.servlet-3.0.0.v201112011016.jar from Sentry dist

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Nov. 30, 2017, 11:05 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64230/
> ---
> 
> (Updated Nov. 30, 2017, 11:05 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2082
> https://issues.apache.org/jira/browse/SENTRY-2082
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The javax.servlet-3.0.0.v201112011016.jar transitive dependency is causing 
> some error exceptions on the Sentry 2.0 distribution in some Mac environments.
> 
> 
> Diffs
> -
> 
>   pom.xml 948f88cff41dc816f083995f795cff6b4b416f28 
> 
> 
> Diff: https://reviews.apache.org/r/64230/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 64002: SENTRY-2068: Disable HTTP TRACE method from the Sentry Web Server

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Nov. 22, 2017, 3:19 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64002/
> ---
> 
> (Updated Nov. 22, 2017, 3:19 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2068
> https://issues.apache.org/jira/browse/sentry-2068
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Disables the HTTP TRACE method by wrapping a constraint that requires 
> authentication when calling such method.
> 
> See more info here:
> http://www.imlc.me/why-we-need-to-disable-trace-method-and-how-to-disable-trace-in-embedded-jetty.html
> https://www.owasp.org/index.php/Cross_Site_Tracing
> https://reformatcode.com/code/http/java-embedded-jetty-is-accepting-http-trace-method
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
>  95b87add5814cc3c0851ca73ca6503306b840594 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java
>  09ee6b4493611c055dd7e96ab8a0b747fd4eb25b 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSSL.java
>  d1d0b4be578ca9b4148a81073a21639cd8688156 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java
>  4a913e5189fa0aea7fb1770eb9f3e8e991289a50 
> 
> 
> Diff: https://reviews.apache.org/r/64002/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Avoiding merge commits to master.

[Alexander Kolbasov]

This is a reminder for developers to avoid merge commits to master. There
might be cases where this is unavoidable, please send a note here if you
think you MUST have a merge commit. Otherwise, please make an effort co
collapse all intermediate changes/merges and create a single commit per
changeset.

Thanks,

- Alex.

Re: [VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

I tried to start Sentry server on Mac and got a weird error . I am running
from the dist directory that I just built with mvn clean install on the
2.0.0 branch.

17/11/29 10:13:25 ERROR thrift.SentryService: Error starting server
java.lang.SecurityException: class "javax.servlet.DispatcherType"'s signer
information does not match signer information of other classes in the same
package
at java.lang.ClassLoader.checkCerts(ClassLoader.java:898)
at java.lang.ClassLoader.preDefineClass(ClassLoader.java:668)
at java.lang.ClassLoader.defineClass(ClassLoader.java:761)
at
java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at
org.apache.sentry.service.thrift.SentryService.startSentryWebServer(SentryService.java:422)
at
org.apache.sentry.service.thrift.SentryService.runServer(SentryService.java:268)
at
org.apache.sentry.service.thrift.SentryService.call(SentryService.java:198)
at
org.apache.sentry.service.thrift.SentryService.call(SentryService.java:76)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.util.concurrent.ExecutionException:
java.lang.Exception: Error starting server
at java.util.concurrent.FutureTask.report(FutureTask.java:122)
at java.util.concurrent.FutureTask.get(FutureTask.java:192)
at
org.apache.sentry.service.thrift.SentryService$CommandImpl.run(SentryService.java:591)
at org.apache.sentry.SentryMain.main(SentryMain.java:122)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.apache.hadoop.util.RunJar.run(RunJar.java:234)
at org.apache.hadoop.util.RunJar.main(RunJar.java:148)
Caused by: java.lang.Exception: Error starting server
at
org.apache.sentry.service.thrift.SentryService.call(SentryService.java:202)
at
org.apache.sentry.service.thrift.SentryService.call(SentryService.java:76)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.SecurityException: class
"javax.servlet.DispatcherType"'s signer information does not match signer
information of other classes in the same package
at java.lang.ClassLoader.checkCerts(ClassLoader.java:898)
at java.lang.ClassLoader.preDefineClass(ClassLoader.java:668)
at java.lang.ClassLoader.defineClass(ClassLoader.java:761)
at
java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
at
org.apache.sentry.service.thrift.SentryService.startSentryWebServer(SentryService.java:422)
at
org.apache.sentry.service.thrift.SentryService.runServer(SentryService.java:268)
at
org.apache.sentry.service.thrift.SentryService.call(SentryService.java:198)
... 5 more

On Tue, Nov 28, 2017 at 4:38 PM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> This is the release of Apache Sentry, version 2.0.0.
>
> It fixes the following issues:
> *https://issues.apache.org/jira/projects/SENTRY/versions/12341081
> 

Re: [VOTE] Release Sentry 2.0.0 RC2

[Alexander Kolbasov]

Would you mind submitting the vote on Monday? This will give people 72 working 
hours to look at it.

Thanks,

- Alex.

> On Dec 1, 2017, at 11:11 PM, Kalyan Kumar Kalvagadda  
> wrote:
> 
> This is the release of Apache Sentry, version 2.0.0.
> 
> It fixes the following issues:
> *https://issues.apache.org/jira/projects/SENTRY/versions/12341081
> *
> 
> Maven artifacts are available
> here:https://repository.apache.org/content/repositories/orgapachesentry-1008
> 
> 
> 
> Tag to be voted on
> *https://git-wip-us.apache.org/repos/asf/sentry/?p=sentry.git;a=tag;h=refs/tags/release-2.0.0
> *
> 
> Sentry's KEYS containing the PGP key we used to sign the release:
> http://www.apache.org/dist/sentry/KEYS
> 
> we are voting on the source:tag=release-2.0.0, SHA=
> c1ca5b22ba8a3a34398e0d62802cf65cfc47b4d1
> (You can get the hash of the tag by doing "git rev-list release-2.0.0 |
> head -n 1" )
> 
> Vote will be open for 72 hours.
> 
> [ ] +1 approve
> [ ] +0 no opinion
> [ ] -1 disapprove (and reason why)
> 
> -Kalyan

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

[Alexander Kolbasov]

> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
> > Line 45 (original), 49 (patched)
> > <https://reviews.apache.org/r/64259/diff/2/?file=1912668#file1912668line49>
> >
> > Do you actually use these classes now? I think you only check for valid 
> > command string which you do in the switch() statement below, so I guess 
> > this can be removed now.
> 
> Xinran Tinney wrote:
> I think it probably still going to be needed since on line 67: 
> options.addOption(null, COMMAND, true, "Command to run. Options: " + 
> COMMANDS.keySet()); 
> what do you think?

The only thing that is used is a list f commands to show - you may just have 
the list of valid commands to preserve this. Or just print static string - it 
doesn't matter much here IMO.


- Alexander


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


On Dec. 11, 2017, 8:03 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> ---
> 
> (Updated Dec. 11, 2017, 8:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TheSentryMain class currently works by mapping the command name to a Java 
> class that is then dynamically loaded:
> String commandName = commandLine.getOptionValue(COMMAND);
> String commandClazz = COMMANDS.get(commandName);
> Object command;
> try {
>   command = Class.forName(commandClazz.trim()).newInstance();
> } catch (Exception e) {
>   String msg = "Could not create instance of " + commandClazz + " for 
> command " + commandName;
>   throw new IllegalStateException(msg, e);
> }
> if (!(command instanceof Command)) {
>   String msg = "Command " + command.getClass().getName() + " is not an 
> instance of "
>   + Command.class.getName();
>   throw new IllegalStateException(msg);
> }
> ((Command)command).run(commandLine.getArgs());
>   }
> This ia too complicated and causes subtle problems at runtime. Instead it 
> should just create a new instance of appropriate class and call it directly.
> 
> 
> Diffs
> -
> 
>   bin/config_tool 4da85673 
>   bin/run_sentry.sh d58d5e5c 
>   bin/sentry 54e545aa 
>   pom.xml dd408d85 
>   sentry-command-line-tools/pom.xml PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  3a981b2a 
> 
> 
> Diff: https://reviews.apache.org/r/64259/diff/2/
> 
> 
> Testing
> ---
> 
> mvn clean install, on cloudcat and all SUCCESS
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Lines 222 (patched)
<https://reviews.apache.org/r/64545/#comment272155>

We log relinguishing the leader status below. Here we are still the leader, 
so the log may be a bit misleading.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Line 254 (original), 255 (patched)
<https://reviews.apache.org/r/64545/#comment272152>

IncarnationId isn't very useful here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Line 262 (original), 263 (patched)
<https://reviews.apache.org/r/64545/#comment272153>

incarnationId isn't very useful here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Line 266 (original), 268 (patched)
<https://reviews.apache.org/r/64545/#comment272156>

Drop incarnationId from here


- Alexander Kolbasov


On Dec. 12, 2017, 5:10 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 12, 2017, 5:10 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles()

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Line 260 (original), 264 (patched)
<https://reviews.apache.org/r/64661/#comment272804>

Nit: move it to the end where it is actually used



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Line 267 (original), 271 (patched)
<https://reviews.apache.org/r/64661/#comment272805>

I think there is a way to directly test for membership in the JDOQL - can 
you check this?

We have similar code in SentryStore which does that.


- Alexander Kolbasov


On Dec. 18, 2017, 4:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> ---
> 
> (Updated Dec. 18, 2017, 4:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to {{getGroupsByRoles()}} function in {{DelegateSentryStore}}.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  4cb46abc5 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

[Alexander Kolbasov]

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




bin/config_tool
Line 55 (original), 55 (patched)
<https://reviews.apache.org/r/64259/#comment272636>

Please verify that this works.



bin/run_sentry.sh
Line 23 (original), 23 (patched)
<https://reviews.apache.org/r/64259/#comment272637>

Does this work?



sentry-command-line-tools/pom.xml
Lines 47 (patched)
<https://reviews.apache.org/r/64259/#comment272640>

You may consider using this instead of the two log4j dependencies. You 
should provide correct version though.



org.slf4j
slf4j-log4j12
1.8.0-beta0




sentry-command-line-tools/pom.xml
Lines 60 (patched)
<https://reviews.apache.org/r/64259/#comment272641>

I'm curios, what uses this?



sentry-command-line-tools/pom.xml
Lines 62 (patched)
<https://reviews.apache.org/r/64259/#comment272642>

I noticed that there is 1.5.4 version of commons-pool which you get from 
hive-metastore and 2.4.2 that you get directly. Are there any potential issues 
with this?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 18 (original), 18 (patched)
<https://reviews.apache.org/r/64259/#comment272644>

You moved the class and now package name doesn't correspond to the path - I 
am not sure whether this can cause any problems. What do you think?

May be you can create package org.apache.sentry here?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 45 (original), 49 (patched)
<https://reviews.apache.org/r/64259/#comment272646>

Do you actually use these classes now? I think you only check for valid 
command string which you do in the switch() statement below, so I guess this 
can be removed now.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 79 (original), 83 (patched)
<https://reviews.apache.org/r/64259/#comment272652>

Since you are touching this, can you put 
`"log4j.category.DataNucleus.Query"` in constant?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 98 (original), 102 (patched)
<https://reviews.apache.org/r/64259/#comment272648>

It would be better to print explicit message telling that command name is 
missing if commandName is null



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 106 (original), 110 (patched)
<https://reviews.apache.org/r/64259/#comment272649>

I don't think you need this now.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 110 (original), 114 (patched)
<https://reviews.apache.org/r/64259/#comment272645>

It is Command, not Object



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Lines 126 (patched)
<https://reviews.apache.org/r/64259/#comment272654>

Add `break`


- Alexander Kolbasov


On Dec. 11, 2017, 8:03 p.m., Xinran Tinney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -------
> 
> (Updated Dec. 11, 2017, 8:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TheSentryMain class currently works by mapping the command name to a Java 
> class that is then dynamically loaded:
> String commandName = commandLine.getOptionValue(COMMAND);
> String commandClazz = COMMANDS.get(commandName);
> Object command;
> try {
>   command = Class.forName(commandClazz.trim()).newInstance();
> } catch (Exception e) {
>   String msg = "Could not create instance of " + commandClazz + " for 
> command " + commandName;
>   throw new IllegalStateException(msg, e);
> }
> if (!(command instanceof Command)) {
>   String msg = "Command " + command.getClass().getName() + " is not an 
> instance of "
>   + Command.class.getName();
>   throw new IllegalStateException(msg);
> }
> ((Command)command).run(commandLine.getArgs());
>   }
> This ia too complicated and causes subtle problems at runtime. Instead it 
> should just create a new instance of appropriate class and call it directly.
> 
> 
> Diffs
> -
> 
>   bin/config_tool 4da85673 
>   bin/run_sentry.sh d58d5e5c 
>   bin/sentry 54e545aa 
&

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 14, 2017, 1:54 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 14, 2017, 1:54 a.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/7/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Line 281 (original), 282 (patched)
<https://reviews.apache.org/r/64545/#comment272308>

Note that in single node mode none of these are relevant since it is always 
a leader.


- Alexander Kolbasov


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: 1.7.1 version in JIRA

[Alexander Kolbasov]

Done


On Wed, Dec 13, 2017 at 6:26 AM, Colm O hEigeartaigh 
wrote:

> Can someone add a 1.7.1 version in JIRA please?
>
> Colm.
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

[Alexander Kolbasov]

> On Dec. 12, 2017, 1:26 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
> > Line 57 (original), 56 (patched)
> > <https://reviews.apache.org/r/63596/diff/6/?file=1912694#file1912694line57>
> >
> > Not necessarily as part of this fix it would be great to have tesst 
> > that not only check counts but actually verify the content of the returned 
> > values.
> 
> Arjun Mishra wrote:
> Sasha, did you mean for it to be "Fix It"? I chose not to make changes to 
> this test cos the purpose of the ticket was to eliminate any references to 
> retrieveFullPathsImage() method. If you meant for this to be "Fix It" please 
> let me know, else I will ask for someone to commit this.

No, I don't think you need to fix it as part of this change, but it would be 
good to add tests later that test the actual content of the snapshot.


- Alexander


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


On Dec. 11, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> -------
> 
> (Updated Dec. 11, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c730a03a5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

> On Dec. 13, 2017, 6:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 281 (original), 282 (patched)
> > <https://reviews.apache.org/r/64545/diff/5-6/?file=1915545#file1915545line282>
> >
> > Note that in single node mode none of these are relevant since it is 
> > always a leader.
> 
> Arjun Mishra wrote:
> In that case it would print the defaults. "isSingleNodeMode=true, 
> incarnationId="", isLeader=true, leaderCount=0". I don't think that's 
> misleading.

Wouldn't it be more useful to print some kind of special status string in this 
case? Something like "Leader election disabled" or whatever you think is useful?


- Alexander


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


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Cutting sentry 2.0.0 off master

[Alexander Kolbasov]

We can still create 2.0 branch and just be more selective about what goes
there - we may choose to skip some commits from master.

- Alex.

On Fri, Nov 17, 2017 at 12:24 PM, Sergio Pena 
wrote:

> +1
>
> Sounds good Kalyan. I'm still finding the test failures and seems related
> to a Jenkins environment more than our code.
>
> On Fri, Nov 17, 2017 at 11:51 AM, Colm O hEigeartaigh  >
> wrote:
>
> > SENTRY-1812 is complete and waiting for review (but currently dependent
> on
> > SENTRY-2012 getting merged first). SENTRY-2012 was merged to master
> today,
> > but I had to revert it as it broke the Kafka tests.
> >
> > Colm.
> >
> > On Fri, Nov 17, 2017 at 5:28 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > Colm/Mano Kovacs,
> > >
> > > Do you have an ETA on SENTRY-1812 and SENTRY-2012?
> > >
> > > -Kalyan
> > >
> > > -Kalyan
> > >
> > > On Fri, Nov 17, 2017 at 11:00 AM, Colm O hEigeartaigh <
> > cohei...@apache.org
> > > > wrote:
> > >
> > >> Fine with me, but I'd like to see the fixes for SENTRY-1812 and
> > >> SENTRY-2012
> > >> make it in to 2.0.0 as well.
> > >>
> > >> Colm.
> > >>
> > >> On Fri, Nov 17, 2017 at 4:56 PM, Kalyan Kumar Kalvagadda <
> > >> kkal...@cloudera.com> wrote:
> > >>
> > >> > Hello all,
> > >> >
> > >> >
> > >> > All of you are aware that we have decided to release sentry 2.0.0.
> As
> > a
> > >> > first step I propose the cut the branch for sentry 2.0.0 off the
> > master
> > >> > today.
> > >> > We still have couple of issues that are pending to for  2.0.0 but I
> > >> would
> > >> > like to cut it as the tests on the master have become flaky.
> > >> > Here are jira's that are blocking sentry 2.0.0 release
> > >> >
> > >> >
> > >> >- *Solr*
> > >> >
> > >> >
> > >> >1. SENTRY-2042  Support file based Sentry provider for Solr
> plugin
> > >> >   2. SENTRY-1480 A upgrade tool to migrate Solr/Sentry
> permissions
> > >> >
> > >> >
> > >> >
> > >> >
> > >> > * - Hive *
> > >> >
> > >> >1. SENTRY-1640 Implement HMS Notification barrier on the HMS
> plugin
> > >> side
> > >> >   2. SENTRY-2048 Bump Hive version to 2.3.2
> > >> >
> > >> > If we cut the branch, we can limit commits to sentry 2.0.0 to that
> are
> > >> > needed and isolate the root cause for the test failures and
> stabilize
> > >> 2.0.0
> > >> > branch.
> > >> >
> > >> > -Kalyan
> > >> >
> > >>
> > >>
> > >>
> > >> --
> > >> Colm O hEigeartaigh
> > >>
> > >> Talend Community Coder
> > >> http://coders.talend.com
> > >>
> > >
> > >
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>

Interesting coincedence

[Alexander Kolbasov]

   1. SENTRY-2000  is"Cut
   2.0.0 branch" !

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 283 (patched)
<https://reviews.apache.org/r/63975/#comment269413>

Why do we need an extra boolean Can we just check for currDB == null ? Or 
it isn't null in this case?


- Alexander Kolbasov


On Nov. 21, 2017, 3:32 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> ---
> 
> (Updated Nov. 21, 2017, 3:32 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at 
> HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e4620ea 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
>  aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/1/
> 
> 
> Testing
> ---
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63874: SENTRY-1812 - Provide interactive Sentry CLI

[Alexander Kolbasov]

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


Fix it, then Ship it!




LGTM, a few nits below.


bin/sentryCli
Lines 21 (patched)
<https://reviews.apache.org/r/63874/#comment269540>

This can be written as 

export SENTRY_HOME=${SENTRY_HOME:-${MYHOME}}



bin/sentryCli
Lines 27 (patched)
<https://reviews.apache.org/r/63874/#comment269541>

Use [[ instead of [



bin/sentryCli
Lines 32 (patched)
<https://reviews.apache.org/r/63874/#comment269544>

Use  `[[ -z ${HADOOP_HOME} ]` instead to test for empty var



bin/sentryCli
Lines 38 (patched)
<https://reviews.apache.org/r/63874/#comment269546>

Use [[



bin/sentryCli
Lines 45 (patched)
<https://reviews.apache.org/r/63874/#comment269548>

Use `+=` to append strings



bin/sentryCli
Lines 53 (patched)
<https://reviews.apache.org/r/63874/#comment269551>

Use `+=`



sentry-tools/src/main/java/org/apache/sentry/shell/SentryCli.java
Lines 117 (patched)
<https://reviews.apache.org/r/63874/#comment269554>

Why is the code parsing commands twice - here and above at line 104?



sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java
Lines 49 (patched)
<https://reviews.apache.org/r/63874/#comment269552>

Nit: Use all caps for enums and don't put them on a single line.



sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java
Lines 279 (patched)
<https://reviews.apache.org/r/63874/#comment269555>

Use switch() for comparing against enums.



sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java
Lines 291 (patched)
<https://reviews.apache.org/r/63874/#comment269556>

Use switch() for comparing against enums


- Alexander Kolbasov


On Nov. 20, 2017, 3:05 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63874/
> ---
> 
> (Updated Nov. 20, 2017, 3:05 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1812
> https://issues.apache.org/jira/browse/SENTRY-1812
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> A new interactive CLI to examine Sentry roles, groups, privileges, etc. 
> Extended Alexander's work to add support for Kafka, Solr, Sqoop, etc.
> 
> 
> Diffs
> -
> 
>   LICENSE.txt e6be7872 
>   bin/sentryCli PRE-CREATION 
>   pom.xml d8636274 
>   sentry-dist/pom.xml 69f4fcca 
>   sentry-tools/pom.xml PRE-CREATION 
>   sentry-tools/src/main/java/org/apache/sentry/shell/GroupShell.java 
> PRE-CREATION 
>   sentry-tools/src/main/java/org/apache/sentry/shell/PrivsShell.java 
> PRE-CREATION 
>   sentry-tools/src/main/java/org/apache/sentry/shell/RolesShell.java 
> PRE-CREATION 
>   sentry-tools/src/main/java/org/apache/sentry/shell/SentryCli.java 
> PRE-CREATION 
>   sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63874/diff/3/
> 
> 
> Testing
> ---
> 
> Tested the CLI against the Sentry service.
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Inconsistent commit messages

[Alexander Kolbasov]

Hello,

I noticed that recently our commit messages became veru inconsistent:

The format we used to have:

SENTRY-2014: incorrect handling of HDFS paths with multiple forward slashes
(Vadim Spector, reviewed by Sergio Pena and Arjun Mishra)

SENTRY-2015 - Refactor Command implementations
  - Reviewed by Sergio Pena

Here reviewer is in the second line

SENTRY-2013 - Align the SentryGenericServiceClient and
SentryPolicyServiceClient a bit more closely
- Signed off by Kalyan.

Here there is no reviewer by it has "Signed off',

SENTRY-2017: Fix Sentry e2e tests to use
SentryMetastorePostEventListenerNotificationLog.

No committer or reviewers here

I think we should agree on one standard format, document it and follow it
for all commits.

- Alex.

Re: Inconsistent commit messages

[Alexander Kolbasov]

Sounds like we have a consensus - use the format

SENTRY-123: Fix important feature (Hacker Master, reviewed by Foo Bar and
Ben Hur)
Any other comments


On Fri, Nov 3, 2017 at 11:36 AM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> Agree.
>
> I've seen the following format repeated in other Apache components:
>
> *SENTRY-2026: Bump Hadoop version from 2.7.2 to 2.7.4 (Na Li, reviewed by
> Sergio Pena)*
>
> That is really helpful. We should use that to know who review the code and
> who is the author of the code as well.
>
> On Wed, Nov 1, 2017 at 10:41 PM, Na Li <lina...@cloudera.com> wrote:
>
> > Sasha,
> >
> > I agree we should have consistent format. It is better to include author,
> > then followed by reviewer. So we can have all information at a glance.
> >
> > Lina
> >
> > On Wed, Nov 1, 2017 at 8:04 PM, Alexander Kolbasov <ak...@cloudera.com>
> > wrote:
> >
> > > Hello,
> > >
> > > I noticed that recently our commit messages became veru inconsistent:
> > >
> > > The format we used to have:
> > >
> > > SENTRY-2014: incorrect handling of HDFS paths with multiple forward
> > slashes
> > > (Vadim Spector, reviewed by Sergio Pena and Arjun Mishra)
> > >
> > > SENTRY-2015 - Refactor Command implementations
> > >   - Reviewed by Sergio Pena
> > >
> > > Here reviewer is in the second line
> > >
> > > SENTRY-2013 - Align the SentryGenericServiceClient and
> > > SentryPolicyServiceClient a bit more closely
> > > - Signed off by Kalyan.
> > >
> > > Here there is no reviewer by it has "Signed off',
> > >
> > > SENTRY-2017: Fix Sentry e2e tests to use
> > > SentryMetastorePostEventListenerNotificationLog.
> > >
> > > No committer or reviewers here
> > >
> > > I think we should agree on one standard format, document it and follow
> it
> > > for all commits.
> > >
> > > - Alex.
> > >
> >
>

Broken master

[Alexander Kolbasov]

Looks like master is broken now which blocks anyone from doing anything. Do
we know what commit broke it? Do we know how this commit managed to get
through?

Once we know the commit it should be reverted ASAP.

- Alex.

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3469 (patched)
<https://reviews.apache.org/r/63645/#comment267763>

While this may be a useful test, it is better to write specific unit test 
for the String manipulation functions directly.


- Alexander Kolbasov


On Nov. 7, 2017, 9:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63645/
> ---
> 
> (Updated Nov. 7, 2017, 9:16 p.m.)
> 
> 
> Review request for sentry, Sergio Pena and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When retrieving full path image update, we split a path by "/" and create HMS 
> Path entries. However, the leading "/" presence will cause issues because on 
> splitting the value at index 0 will be empty. This will affect the creation 
> of HMS path entries.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
>  cef8bd734 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63645/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 63647: SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors

[Alexander Kolbasov]

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

Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


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


Repository: sentry


Description
---

SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 71eb9c1b6d02326fc9a7b702652253bd9b57d725 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

[Alexander Kolbasov]

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



Do you also want to update PathUpdate.parsePath() to be consistent?


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 219 (patched)
<https://reviews.apache.org/r/63645/#comment267759>

This comment doesn't belong here - this is utility class which knows 
nothing about HMS - it is about string manipulation.

The relevant omment should say that leading slashes are ignored, so 

/a/b/c beomes {a, b, c}



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Line 219 (original), 224 (patched)
<https://reviews.apache.org/r/63645/#comment267756>

This function now does more then split, so it should be updated and 
documented properly



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Line 220 (original), 226 (patched)
<https://reviews.apache.org/r/63645/#comment267760>

Do we really need to chck for null? Can we specify the non-null value as a 
contract?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 229 (patched)
<https://reviews.apache.org/r/63645/#comment267755>

This function isn't used by anything so why make it public? I suggest to 
just inline it in splitPath()

This whole function can be replaced with
path.replaceAll("^/+", "")



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 230 (patched)
<https://reviews.apache.org/r/63645/#comment267762>

We don't need to collapse slashes any more



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 232 (patched)
<https://reviews.apache.org/r/63645/#comment267761>

We don't need to replace all since we do path.split("/+"). We only need to 
remove leading slashes, so this check should be changed to startsWith("/")


- Alexander Kolbasov


On Nov. 7, 2017, 9:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63645/
> ---
> 
> (Updated Nov. 7, 2017, 9:16 p.m.)
> 
> 
> Review request for sentry, Sergio Pena and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When retrieving full path image update, we split a path by "/" and create HMS 
> Path entries. However, the leading "/" presence will cause issues because on 
> splitting the value at index 0 will be empty. This will affect the creation 
> of HMS path entries.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
>  cef8bd734 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63645/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63647: SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors

[Alexander Kolbasov]

> On Nov. 9, 2017, 2:56 p.m., kalyan kumar kalvagadda wrote:
> > What is the issue we are observing when this id is not set?

When timeout exception occurs, id is not set. This is a mandatory field, so 
Thrift creates its own exception which replaces the original timeout exception. 
As a result, client gets the wrong exception information back.


- Alexander


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


On Nov. 7, 2017, 9:58 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63647/
> ---
> 
> (Updated Nov. 7, 2017, 9:58 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2036
> https://issues.apache.org/jira/browse/SENTRY-2036
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
>  71eb9c1b6d02326fc9a7b702652253bd9b57d725 
> 
> 
> Diff: https://reviews.apache.org/r/63647/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 63704: SENTRY-1662 Constants java uses mutable collection

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Nov. 9, 2017, 7:32 p.m., Steve Moist wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63704/
> ---
> 
> (Updated Nov. 9, 2017, 7:32 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1662
> https://issues.apache.org/jira/browse/SENTRY-1662
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Changes the hash maps to be unmodifable maps.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java
>  2e71ce02 
> 
> 
> Diff: https://reviews.apache.org/r/63704/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests ran.
> 
> 
> Thanks,
> 
> Steve Moist
> 
>

Re: Review Request 63667: SENTRY-2037: Remove not needed sentry-binding-hive-v2 dependency from the main pom.xml

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Nov. 8, 2017, 3 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63667/
> ---
> 
> (Updated Nov. 8, 2017, 3 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2037
> https://issues.apache.org/jira/browse/sentry-2037
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Remove the sentry-binding-hive-v2 dependency from the main pom.
> 
> 
> Diffs
> -
> 
>   pom.xml bea2c5fe4bae53eb6b45602f2586ef30e9e7c309 
> 
> 
> Diff: https://reviews.apache.org/r/63667/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63687: SENTRY-2039: KeyValue is case sensitive and it causes incompatibility issues with external comp

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
Lines 35 (patched)
<https://reviews.apache.org/r/63687/#comment268200>

Can you add Javadoc to the class explaining that by contract keys are 
always case-insensitive?


- Alexander Kolbasov


On Nov. 9, 2017, 11:44 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63687/
> ---
> 
> (Updated Nov. 9, 2017, 11:44 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2039
> https://issues.apache.org/jira/browse/sentry-2039
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Return the equalsIgnoreCase() to the KeyValue.equals() method (removed by 
> SENTRY-999)
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2605459ac4166a784bb1d50362509221 
>   
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestKeyValue.java
>  ca44a245dc9abf003eaae13d0ebbfb1ef7eff29a 
>   
> sentry-policy/sentry-policy-common/src/test/java/org/apache/sentry/policy/common/TestCommonPrivilege.java
>  3f60b1901976d2236db0814fd63114c9598e8215 
> 
> 
> Diff: https://reviews.apache.org/r/63687/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63687: SENTRY-2039: KeyValue is case sensitive and it causes incompatibility issues with external comp

[Alexander Kolbasov]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
Line 87 (original), 87 (patched)
<https://reviews.apache.org/r/63687/#comment267962>

Please add javaDoc explaining that this is case-insensitive comparison. 
Should both key and value be case-insensitive?

Would it make sense to instead convert key and value to the canonical case 
in constructor?


- Alexander Kolbasov


On Nov. 8, 2017, 11:43 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63687/
> ---
> 
> (Updated Nov. 8, 2017, 11:43 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2039
> https://issues.apache.org/jira/browse/sentry-2039
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Return the equalsIgnoreCase() to the KeyValue.equals() method (removed by 
> SENTRY-999)
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
>  4e944e5f2605459ac4166a784bb1d50362509221 
>   
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestKeyValue.java
>  ca44a245dc9abf003eaae13d0ebbfb1ef7eff29a 
>   
> sentry-policy/sentry-policy-common/src/test/java/org/apache/sentry/policy/common/TestCommonPrivilege.java
>  3f60b1901976d2236db0814fd63114c9598e8215 
> 
> 
> Diff: https://reviews.apache.org/r/63687/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63415: SENTRY-2012 Make SentryShellGeneric extendible

[Alexander Kolbasov]

r-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 173 (original), 157 (patched)
<https://reviews.apache.org/r/63415/#comment267960>

Looks like this method is never used



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 182 (original), 161 (patched)
<https://reviews.apache.org/r/63415/#comment267961>

can this be package-private?


- Alexander Kolbasov


On Oct. 31, 2017, 2:20 p.m., Mano Kovacs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63415/
> ---
> 
> (Updated Oct. 31, 2017, 2:20 p.m.)
> 
> 
> Review request for sentry and Colm O hEigeartaigh.
> 
> 
> Bugs: SENTRY-2012
> https://issues.apache.org/jira/browse/SENTRY-2012
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch makes SentryShellGeneric and GenericPrivilegeConverter independent 
> of any component.
> 
> 
> Diffs
> -
> 
>   bin/sentryShell 17b1429f096bc70c2dead8dbf708fd039baf319d 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AbstractAuthorizableFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableType.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaAuthorizable.java
>  52ae614b7f44fc351406f5b4ca6d2a631b8ec750 
>   
> sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java
>  45a1148533ec4eb0f506b2dc6c81df6cc1663870 
>   
> sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizable.java
>  5a55963d40a9ad959660cf5381d3c2c6269ab0db 
>   
> sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchModelAuthorizables.java
>  2b190e5ca2b49ca5737a9835e4b301317138ac21 
>   
> sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/SqoopAuthorizable.java
>  934875efbeff9ee66575e6e2a71c069da6e56cc4 
>   
> sentry-core/sentry-core-model-sqoop/src/main/java/org/apache/sentry/core/model/sqoop/SqoopModelAuthorizables.java
>  3bb9a19a36f297c8f6c94c0f6d112873ba2e32fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
>  51d6df958aea7681c62a751f25e3d2d624dc96dd 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java
>  77d39194f548dad8700d6f6ef6330c2fa0b92579 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java
>  49f18c8994df27217592fafc04ca3b96eaa4a978 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSqoop.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryConfigToolSolr.java
>  3685073910e4b5f45183742723c6146749a927f1 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellKafka.java
>  80bbcf184084a0a558351f2872ca0bb922c81aa3 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java
>  55831a45b4e75fefecbebd8ce13da73ce3697d4f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSqoop.java
>  7bafd8c40c0c0be2b91fac4932bdb15709a379fd 
> 
> 
> Diff: https://reviews.apache.org/r/63415/diff/2/
> 
> 
> Testing
> ---
> 
> Unit-tests
> 
> 
> Thanks,
> 
> Mano Kovacs
> 
>

Sentry as a library

[Alexander Kolbasov]

What do you think about providing a ‘sentry service as a library’ which is a 
wrapper that can run inside HMS and other consumers that will talk to the 
underlying database, understand schemas, etc and provide all information to a 
client without going through Thrift interface to the server?

- Alex

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 377 (patched)
<https://reviews.apache.org/r/63881/#comment271311>

Will someone up the call stack print the full stack trace?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 420 (original), 430 (patched)
<https://reviews.apache.org/r/63881/#comment271312>

Use `ID = {}", event.getEventId`.
Also, do we want to show the actual failure here or it will be printed 
elsewhere?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 190 (patched)
<https://reviews.apache.org/r/63881/#comment271313>

Does this work? I am not sure that logger correctly supports more then two 
args. You can pass Array with more then 2 elements though.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 209 (patched)
<https://reviews.apache.org/r/63881/#comment271316>

It is visuall better to rearrange these to show the lower ID first.

It would be nice to see something like `received non-consequitive event 
sequence 10, 12`


- Alexander Kolbasov


On Dec. 5, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 5, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/8/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: [RESULT][VOTE] Release Sentry version 2.0.0

[Alexander Kolbasov]

Thanks Kalyan!


On Thu, Dec 7, 2017 at 11:36 AM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> Voting is now closed and has passed with the following tally,
>
> Binding +1s: Sergio Pena, Alexander Kolbasov, Colm o hEigeartaigh
>
>
> Thanks to everyone who voted! I'll continue with the rest of the
> release process.
>
>
> -Kalyan
>

Re: Review Request 64392: SENTRY-2092: Drop Role log message shows Creating role

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryAccessController.java
Line 147 (original), 147 (patched)
<https://reviews.apache.org/r/64392/#comment271548>

Change to `"Sentry failed to drop role: "`



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
Line 144 (original), 144 (patched)
<https://reviews.apache.org/r/64392/#comment271549>

Change to `"Sentry failed to drop role: "`


- Alexander Kolbasov


On Dec. 6, 2017, 10:20 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64392/
> ---
> 
> (Updated Dec. 6, 2017, 10:20 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the typo in log messages
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryAccessController.java
>  fed483f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  0f93b42 
> 
> 
> Diff: https://reviews.apache.org/r/64392/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: [VOTE] Sentry Release 2.0.0 RC3

[Alexander Kolbasov]

+1

On Tue, Dec 5, 2017 at 9:01 AM, Colm O hEigeartaigh 
wrote:

> +1 from me. Checked digests and signatures, built the tag and source
> distribution, tested the binary distribution.
>
> Colm.
>
> On Tue, Dec 5, 2017 at 4:30 PM, Sergio Pena 
> wrote:
>
> > I verified the following on the RC3:
> >
> >- GPG and checksums verified
> >- Source can be build
> >- NOTICE, LICENSE and CHANGELOG files verified
> >- Install the binary in a local cluster environment with:
> >   - hive 2.3.2
> >   - hadoop 2.7.4
> >   - zookeeper 3.4.5
> >- Sentry HA and Hive authorization are working correctly
> >- Sentry HA failover is working correctly
> >
> > The release binaries are working fine.
> >
> > +1
> >
> > Thanks Kalyan.
> >
> > On Mon, Dec 4, 2017 at 3:18 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > This is the release of Apache Sentry, version 2.0.0.
> > >
> > > It fixes the following issues:
> > > *https://issues.apache.org/jira/projects/SENTRY/versions/12341081
> > > *
> > >
> > > Source and bin files
> > > https://dist.apache.org/repos/dist/dev/sentry/2.0.0/
> > >
> > > Maven artifacts are available
> > > here:https://repository.apache.org/content/
> repositories/orgapachesentry-
> > > 1009
> > >   > orgapachesentry-1008>
> > >
> > >
> > > Tag to be voted on
> > > *https://git-wip-us.apache.org/repos/asf/sentry/?p=
> > > sentry.git;a=tag;h=refs/tags/release-2.0.0
> > >  > > sentry.git;a=tag;h=refs/tags/release-2.0.0>*
> > >
> > > Sentry's KEYS containing the PGP key we used to sign the release:
> > > http://www.apache.org/dist/sentry/KEYS
> > >
> > >  we are voting on the source:tag=release-2.0.0, SHA=
> > > d77edb5056426dbf6106ee068ad5ae176fe0b225
> > >  > sentry.git;a=commit;h=
> > > d77edb5056426dbf6106ee068ad5ae176fe0b225>
> > >  (You can get the hash of the tag by doing "git rev-list release-2.0.0
> |
> > > head -n 1" )
> > >
> > > Vote will be open for a minimum 72 hours.
> > >
> > > [ ] +1 approve
> > > [ ] +0 no opinion
> > > [ ] -1 disapprove (and reason why)
> > >
> > > -Kalyan
> > >
> >
>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Review Request 64392: SENTRY-2092: Drop Role log message shows Creating role

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 6, 2017, 11:42 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64392/
> ---
> 
> (Updated Dec. 6, 2017, 11:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix the typo in log messages
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryAccessController.java
>  fed483f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  0f93b42 
> 
> 
> Diff: https://reviews.apache.org/r/64392/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

[Alexander Kolbasov]

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


Ship it!




Ship It!


sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
Line 57 (original), 56 (patched)
<https://reviews.apache.org/r/63596/#comment272035>

Not necessarily as part of this fix it would be great to have tesst that 
not only check counts but actually verify the content of the returned values.


- Alexander Kolbasov


On Dec. 11, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Dec. 11, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c730a03a5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64241: SENTRY-2081: Update the LICENSE.txt with the license information of distributed jars

[Alexander Kolbasov]

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




LICENSE.txt
Line 214 (original)
<https://reviews.apache.org/r/64241/#comment270795>

Should theLICENSE.txt include copyrights as well or not? Looks like you are 
removing some copyrights.



README_license.txt
Lines 9 (patched)
<https://reviews.apache.org/r/64241/#comment270826>

s/in places/in place



README_license.txt
Lines 10 (patched)
<https://reviews.apache.org/r/64241/#comment270827>

s/build/built



README_license.txt
Lines 11 (patched)
<https://reviews.apache.org/r/64241/#comment270828>

Does it run during every build or it should be manually invoked?



README_license.txt
Lines 12 (patched)
<https://reviews.apache.org/r/64241/#comment270829>

Before you say 'we' and here it is 'you'. Please be consistent



README_license.txt
Lines 14 (patched)
<https://reviews.apache.org/r/64241/#comment270830>

I think third party is two words. Also this doesn't look like an item from 
enumeration, just the description text, so this should be moved to the top 
level paragraph.



README_license.txt
Lines 16 (patched)
<https://reviews.apache.org/r/64241/#comment270831>

with licenses listed below



README_license.txt
Lines 34 (patched)
<https://reviews.apache.org/r/64241/#comment270832>

Is there anything that checks correctness of pointers? What would happen if 
release manager forgets to add the file?



pom.xml
Lines 734 (patched)
<https://reviews.apache.org/r/64241/#comment270821>

This looks like an unrelated change - can you move this to a different JIRA?


- Alexander Kolbasov


On Dec. 1, 2017, 9:29 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64241/
> ---
> 
> (Updated Dec. 1, 2017, 9:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Colm O 
> hEigeartaigh, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2081
> https://issues.apache.org/jira/browse/SENTRY-2081
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> As per https://www.apache.org/dev/licensing-howto.html , sentry should update 
> the LICENSE.txt file with license information of all the jars that sentry is 
> distributing along with the pointer to the LICENSE files of the dependencies.
> 
> 
> Diffs
> -
> 
>   LICENSE.txt b794ae6604774186020a1cf3dde922e92da57276 
>   README_license.txt PRE-CREATION 
>   pom.xml eec185bc6409e5cebee12f3a0e4ca17c843cd631 
>   sentry-dist/pom.xml 4c69535660b132943c7cdc2419fff140a0909a48 
>   sentry-dist/src/license/THIRD-PARTY.ftl PRE-CREATION 
>   sentry-dist/src/license/THIRD-PARTY.properties PRE-CREATION 
>   sentry-dist/src/license/override-THIRD-PARTY.properties PRE-CREATION 
>   sentry-dist/src/main/assembly/bin.xml 
> 5727fc964bc139a5bd5490132efad13db6cbcf44 
>   sentry-dist/src/main/resources/licences/BSD_2-clause.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/BSD_License.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_1_0.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_1_1.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_2.txt PRE-CREATION 
>   
> sentry-dist/src/main/resources/licences/Eclipse_Public_License_-_Version_1_0.txt
>  PRE-CREATION 
>   sentry-dist/src/main/resources/licences/MIT_License.txt PRE-CREATION 
>   
> sentry-dist/src/main/resources/licences/Mozilla_Public_License_Version_1_1.txt
>  PRE-CREATION 
>   sentry-dist/src/main/resources/licences/The_BSD_3-Clause_License.txt 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64241/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure that LICENSE.txt file generated in sentry-dist/target directory has 
> the license informaion of all the jars that sentry is distributing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 64241: SENTRY-2081: Update the LICENSE.txt with the license information of distributed jars

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 1, 2017, 11:12 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64241/
> ---
> 
> (Updated Dec. 1, 2017, 11:12 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Colm O 
> hEigeartaigh, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2081
> https://issues.apache.org/jira/browse/SENTRY-2081
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> As per https://www.apache.org/dev/licensing-howto.html , sentry should update 
> the LICENSE.txt file with license information of all the jars that sentry is 
> distributing along with the pointer to the LICENSE files of the dependencies.
> 
> 
> Diffs
> -
> 
>   LICENSE.txt b794ae6604774186020a1cf3dde922e92da57276 
>   README_license.txt PRE-CREATION 
>   pom.xml eec185bc6409e5cebee12f3a0e4ca17c843cd631 
>   sentry-dist/pom.xml 4c69535660b132943c7cdc2419fff140a0909a48 
>   sentry-dist/src/license/THIRD-PARTY.ftl PRE-CREATION 
>   sentry-dist/src/license/THIRD-PARTY.properties PRE-CREATION 
>   sentry-dist/src/license/override-THIRD-PARTY.properties PRE-CREATION 
>   sentry-dist/src/main/assembly/bin.xml 
> 5727fc964bc139a5bd5490132efad13db6cbcf44 
>   sentry-dist/src/main/resources/licences/BSD_2-clause.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/BSD_License.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_1_0.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_1_1.txt PRE-CREATION 
>   sentry-dist/src/main/resources/licences/CDDL_2.txt PRE-CREATION 
>   
> sentry-dist/src/main/resources/licences/Eclipse_Public_License_-_Version_1_0.txt
>  PRE-CREATION 
>   sentry-dist/src/main/resources/licences/MIT_License.txt PRE-CREATION 
>   
> sentry-dist/src/main/resources/licences/Mozilla_Public_License_Version_1_1.txt
>  PRE-CREATION 
>   sentry-dist/src/main/resources/licences/The_BSD_3-Clause_License.txt 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64241/diff/8/
> 
> 
> Testing
> ---
> 
> Made sure that LICENSE.txt file generated in sentry-dist/target directory has 
> the license informaion of all the jars that sentry is distributing.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 64273: SENTRY-2084: Exclude javax.jms:jms from sentry distribution

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


pom.xml
Lines 734 (patched)
<https://reviews.apache.org/r/64273/#comment270851>

Please add comment explaining why this is excluded


- Alexander Kolbasov


On Dec. 1, 2017, 11:19 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64273/
> ---
> 
> (Updated Dec. 1, 2017, 11:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Sergio Pena.
> 
> 
> Bugs: SENTRY-2084
> https://issues.apache.org/jira/browse/SENTRY-2084
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> we need to stop distributing jms artifact with group id: javax.jms as these 
> no valid valid license information available for this jar and secondly it is 
> a transitive dependency which sentry is not dependent on.
> This code change makes sure that we dont distrubute javax.jms:jms
> 
> 
> Diffs
> -
> 
>   pom.xml eec185bc6409e5cebee12f3a0e4ca17c843cd631 
> 
> 
> Diff: https://reviews.apache.org/r/64273/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the jar is not part of the sentry distribution.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 63415: SENTRY-2012 Make SentryShellGeneric extendible

[Alexander Kolbasov]

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



This isn't a full review - just looked at some of the changes - probably 
comments apply in other files as well.

The biggest concern - please stay away from Java8-specific features. Although 
there is decision to move to Java8 for compilation, there is no consensus on 
when should Java8 specific changes be allowed.


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java
Lines 21 (patched)
<https://reviews.apache.org/r/63415/#comment266842>

Please provide documentation for this class - it is pretty basioc, so 
should have very good documentation.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/AuthorizableFactory.java
Lines 26 (patched)
<https://reviews.apache.org/r/63415/#comment266844>

I think KeyValue is a useless class which shouldn't be used by more things. 
We should either use Pair or something specifically useful for Sentry.

Part of that, we shouldn't include enforce parsing in the API, so the API 
should get both key and values as separate entities.



sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java
Lines 25 (patched)
<https://reviews.apache.org/r/63415/#comment266858>

Since create(type, name) can be static, you don't really need an instance 
object



sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java
Line 24 (original), 27 (patched)
<https://reviews.apache.org/r/63415/#comment266854>

Please mark this as @Override



sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java
Line 35 (original), 38 (patched)
<https://reviews.apache.org/r/63415/#comment266855>

This one throws COnfigurationException while the original interface didn't



sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaModelAuthorizables.java
Line 39 (original), 54 (patched)
<https://reviews.apache.org/r/63415/#comment266857>

Please use a different name here - use of overloading here is confusing.

Also, shouldn't this be a static method?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Lines 57 (patched)
<https://reviews.apache.org/r/63415/#comment266851>

This is Java-8 only feature - we do not have consensus on using any Java8 
features in Sentry code. FOr now please stick to Java 7 features.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 116 (original), 110 (patched)
<https://reviews.apache.org/r/63415/#comment266845>

Since you are changing the code, can you also convert it to foreach 
construct?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 138 (original), 132 (patched)
<https://reviews.apache.org/r/63415/#comment266850>

Do we have a case of parser that is not set? What does it mean to "parse" a 
privilege string - please add doc explaining what this function is supposed to 
do.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 161 (original), 150 (patched)
<https://reviews.apache.org/r/63415/#comment266846>

Why does it throws an exception? Do you need it - looks like it is only 
used here - why not use privilegeValidators directly?

If you want to make it protected, please add documentation



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 173 (original), 158 (patched)
<https://reviews.apache.org/r/63415/#comment266847>

Why do you need a one-liner function here - can you just inline the call?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 177 (original), 162 (patched)
<https://reviews.apache.org/r/63415/#comment266849>

This seems to be unused - why is it defined?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/GenericPrivilegeConverter.java
Line 182 (original), 166 (patched)
<https://reviews.apache.org/r/63415/#comment266848>

Is there any reason not to set it in constructor?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
Lines 40 (patched)
<https://reviews.apache.org/r/63415/#comment266852>

Please do not use la

Re: [VOTE] - Release Apache Sentry 1.7.1

[Alexander Kolbasov]

+1

On Wed, Dec 20, 2017 at 7:04 AM, Sergio Pena 
wrote:

> Agree. I checked the rest of the release (checksums, gpg, build) and it is
> good.
>
> +1
>
> On Wed, Dec 20, 2017 at 2:51 AM Colm O hEigeartaigh 
> wrote:
>
> > Yeah I saw that afterwards + updated the branch. I didn't think it was
> > worthy of cancelling the vote though. What do others think?
> >
> > Colm.
> >
> > On Wed, Dec 20, 2017 at 12:29 AM, Sergio Pena 
> > wrote:
> >
> >> Colm, the CHANGELOG does not have the 1.7.1 fixes.
> >> Do you need to add those?
> >>
> >> Sergio
> >>
> >> On Mon, Dec 18, 2017 at 8:58 AM, Colm O hEigeartaigh <
> cohei...@apache.org
> >> > wrote:
> >>
> >>> This is a vote to release Apache Sentry 1.7.1.
> >>>
> >>> Artifacts:
> >>>
> >>> https://dist.apache.org/repos/dist/dev/sentry/1.7.1/
> >>>
> >>> Git tag:
> >>>
> >>> https://github.com/apache/sentry/tree/release-1.7.1
> >>>
> >>> Issues fixed:
> >>>
> >>> https://issues.apache.org/jira/projects/SENTRY/versions/12342308
> >>>
> >>> +1 from me.
> >>>
> >>> Colm.
> >>>
> >>> --
> >>> Colm O hEigeartaigh
> >>>
> >>> Talend Community Coder
> >>> http://coders.talend.com
> >>>
> >>
> >>
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>

Re: Review Request 67174: SENTRY-2156: Update provider-db backend code to grant privileges to user

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
Lines 303 (patched)
<https://reviews.apache.org/r/67174/#comment285425>

Please do not add undocumented public methods.
Do you really need this? Can callers decide which method to call instead - 
they can call either addUsersFilter or addRolesFilter.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 373 (patched)
<https://reviews.apache.org/r/67174/#comment285426>

Is there a value in retrying this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 375 (patched)
<https://reviews.apache.org/r/67174/#comment285427>

Can you use lambda-style instead similar to the way other methods do it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 796 (patched)
<https://reviews.apache.org/r/67174/#comment285430>

First sentence should end with a dot. This talks about doing something to 
the entity's privileges, but there is no entity parameter - which one is the 
entity?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 798 (patched)
<https://reviews.apache.org/r/67174/#comment285431>

typo: input.
Are existing privileges just removed or replaced with ALL?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 803 (patched)
<https://reviews.apache.org/r/67174/#comment285432>

This is a predicate, so probably it should be called isSomething...



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 812 (patched)
<https://reviews.apache.org/r/67174/#comment285434>

you can use 

```Sets.newHashSet("a", "b", "c")```



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 819 (patched)
<https://reviews.apache.org/r/67174/#comment285435>

It is better to have preconditions check upfront - there is no need to do 
extra work if preconditions fail.

You can also put @NotNull annotation - it will document not null 
requirements and enable automatic checking.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 858 (patched)
<https://reviews.apache.org/r/67174/#comment285436>

It may be easier to read if you modify the if statement above:

```if (...) {
   return true;
   }
   
   // handle the more complicated case
```


- Alexander Kolbasov


On May 17, 2018, 1:28 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67174/
> -------
> 
> (Updated May 17, 2018, 1:28 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Bugs: sentry-2156
> https://issues.apache.org/jira/browse/sentry-2156
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add functions related to grant/revoke privileges to/from user
> 
> Fix the bugs related to grant/revoke partial privileges. They are caused by 
> adding fine grained privileges (CRATE, DROP, ALTER)
> add test cases for grant/revoke privileges to/from user
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  71e9585 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  816cfe1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  8a77fc1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  cafe2b5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  0322cc3 
> 
> 
> Diff: https://reviews.apache.org/r/67174/diff/2/
> 
> 
> Testing
> ---
> 
> all test cases in TestSentryStore passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Thoughts on using lombok?

[Alexander Kolbasov]

Brian,

This seems interesting. How does it interplay with IntelliJ/Eclipse ability to 
find all usages of the method - will we still be able to find all references to 
constructors/getters/setters e.t.c. ?

> On May 24, 2018, at 12:54 PM, Brian Towles  wrote:
> 
> Howdy all,
> 
> I was wondering what the teams thoughts are on using Lombok?
> 
> https://projectlombok.org/
> 
> Lombok is a tool that uses annotations at *compile time* to dynamically
> generate the boilerplate code for things like getters, setters, basic
> constructors, toString, equals, etc...
> There are no actual code changes to the source, so its not a traditional
> generator.  It uses the internal
> To me it has been a very helpful time saver for coding and maintenance and
> it allows for us to avoid missing things.
> 
> A good look at how it works in IntelliJ and Eclipse is here:
> https://www.javacodegeeks.com/2016/06/lombok-compile-time-java-annotation-preprocessor-minimize-code-size.html
> 
> But as a quick example of using this for a very simple class and a single
> @Data annotation takes:
> 
> package org.apache.sentry;
> 
> import lombok.Data;
> 
> @Data
> public class SimpleBean {
>  private final String name;
>  private int age;
>  private double score;
>  private String[] tags;
> }
> 
> and turn it effectively into
> 
> package org.apache.sentry;
> 
> public class SimpleBean {
>  private final String name;
>  private int age;
>  private double score;
>  private String[] tags;
> 
>  @java.beans.ConstructorProperties({"name"})
>  public SimpleBean(String name) {
>this.name = name;
>  }
> 
>  public String getName() {
>return this.name;
>  }
> 
>  public int getAge() {
>return this.age;
>  }
> 
>  public double getScore() {
>return this.score;
>  }
> 
>  public String[] getTags() {
>return this.tags;
>  }
> 
>  public void setAge(int age) {
>this.age = age;
>  }
> 
>  public void setScore(double score) {
>this.score = score;
>  }
> 
>  public void setTags(String[] tags) {
>this.tags = tags;
>  }
> 
>  public boolean equals(Object o) {
>if (o == this) {
>  return true;
>}
>if (!(o instanceof SimpleBean)) {
>  return false;
>}
>final SimpleBean other = (SimpleBean) o;
>if (!other.canEqual((Object) this)) {
>  return false;
>}
>final Object this$name = this.getName();
>final Object other$name = other.getName();
>if (this$name == null ? other$name != null :
> !this$name.equals(other$name)) {
>  return false;
>}
>if (this.getAge() != other.getAge()) {
>  return false;
>}
>if (Double.compare(this.getScore(), other.getScore()) != 0) {
>  return false;
>}
>if (!java.util.Arrays.deepEquals(this.getTags(), other.getTags())) {
>  return false;
>}
>return true;
>  }
> 
>  public int hashCode() {
>final int PRIME = 59;
>int result = 1;
>final Object $name = this.getName();
>result = result * PRIME + ($name == null ? 43 : $name.hashCode());
>result = result * PRIME + this.getAge();
>final long $score = Double.doubleToLongBits(this.getScore());
>result = result * PRIME + (int) ($score >>> 32 ^ $score);
>result = result * PRIME + java.util.Arrays.deepHashCode(this.getTags());
>return result;
>  }
> 
>  protected boolean canEqual(Object other) {
>return other instanceof SimpleBean;
>  }
> 
>  public String toString() {
>return "SimpleBean(name=" + this.getName() + ", age=" +
> this.getAge() + ", score=" + this
>.getScore() + ", tags=" +
> java.util.Arrays.deepToString(this.getTags()) + ")";
>  }
> }
> 
> 
> There are more helper functions in the library for things like Builders,
> and Synchronized.
> 
> What do you guys think about using this in Sentry?
> 
> -=Brian
> 
> -- 
> *Brian Towles* | Software Engineer
> t. (512) 415- <00>8105 e. btow...@cloudera.com 
> cloudera.com 
> 
> [image: Cloudera] 
> 
> [image: Cloudera on Twitter]  [image:
> Cloudera on Facebook]  [image: Cloudera
> on LinkedIn] 
> --

Re: Sentry Release Schedule for FGP and ABAC

[Alexander Kolbasov]

Stephen,

a lot depends on your plans in terms of breaking functionality. For
example, one of the reasons Sentry HA was developed on a feature branch was
because it was a serious change in architecture and in broke functionality
for a while. I think some of the merge problems which Sergio referred to
were caused by poor planning and communication - I think we are in much
better shape now.

One thing I would be concerned (in case you do your development in master
branch) is that we end up shipping a release with half-baked feature where
there is a bunch of things that are there for the future but not really
used. If you think this isn't a really a problem, developing on master is
fine since it will automatically handle any potential conflicts with
fine-grained privileges changes.

Alex

On Thu, Apr 26, 2018 at 9:08 AM, Stephen Moist  wrote:

> Hey all, what does the current roadmap and release schedule look like for
> FGP and ABAC?  I’ve been told that FGP is going out in the next release,
> ABAC is more slated for the summer.  How do we want to handle simultaneous
> development of these features?  For ABAC, our dev process is more agile.
> So while we have a working version of ABAC right now in review, it’s not
> the final solution.  We plan to iterate, improve, fix and add features to
> it over the next few months.  I had talked with Kalyan and Sergio offline
> once, they don’t like large patches and recommended not using a feature
> branch.  I don’t see an issue with continuing to develop ABAC and FGP at
> the same time and committing both to master.  We’ll add a switch in ABAC to
> turn the feature off for now through the next release.  What does the
> community think about supporting development of two different features at
> once?

Organizing JIRAs as subtasks for ABAC and FGP

[Alexander Kolbasov]

It is rather difficult to navigate multiple JIRAs associated with ABAC and
FGP.

I wold suggest moving all jiras as subtask of two uber-jiras - one for FGP
and one for ABAC. What do you think?

- Alex

Re: HMS catalogs and Sentry

[Alexander Kolbasov]

I think the way Alan Gates describes it is that the reason is coexistence
of different databases within HMS for example Kafka may store its metadata
within 'kafka' catalog, etc. The data within each catalog seems to be the
same though - it is still databases, tables and partitions. There is some
discussion and explanation in the JIRA.

On Thu, Apr 26, 2018 at 10:21 AM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> I feel, we need to understand it better before considering to put in sentry
> wish list.
>
>
> *Thanks,Kalyan Kumar Kalvagadda* | Software Engineer
> t. (469) 279- <00>5732
> cloudera.com <https://www.cloudera.com>
>
> [image: Cloudera] <https://www.cloudera.com/>
>
> [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image:
> Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: Cloudera
> on LinkedIn] <https://www.linkedin.com/company/cloudera>
> --
>
> On Thu, Apr 26, 2018 at 11:31 AM, Na Li <lina...@cloudera.com> wrote:
>
> > Sasha,
> >
> > The jira  https://issues.apache.org/jira/browse/HIVE-18755 has no
> > description on what's the motivation of having catalog and how it works.
> >
> > Is catalog similar to namespace, so all databases in a catalog can be
> > managed by one entity?
> >
> > Do you know any description that would help us understand how it work
> other
> > than read the hive code?
> >
> > Thanks,
> >
> > Lina
> >
> > On Thu, Apr 26, 2018 at 10:47 AM, Sergio Pena <sergio.p...@cloudera.com>
> > wrote:
> >
> > > What is the reason for having catalogs in HMS? I was looking at the
> JIRA
> > > but I don't see how catalogs will be used.
> > >
> > > Nevertheless, if HMS is going to support catalogs, then we should
> support
> > > them when we decide to move to Hive 3.0 at some point.
> > > Should we put this on our wishlist for Sentry?
> > >
> > > On Wed, Apr 25, 2018 at 7:45 PM, Alexander Kolbasov <
> ak...@cloudera.com>
> > > wrote:
> > >
> > > > See https://issues.apache.org/jira/browse/HIVE-18755. Basically
> > catalog
> > > is
> > > > a container for databases - all databases belong to a catalog. This
> > means
> > > > that Sentry may need to support catalog-level privileges.
> > > >
> > > > - Alex
> > > >
> > > > On Wed, Apr 25, 2018 at 5:06 PM, Kalyan Kumar Kalvagadda <
> > > > kkal...@cloudera.com> wrote:
> > > >
> > > > > Sasha,
> > > > >
> > > > > Could you share information on what catalog is?
> > > > >
> > > > > -Kalyan
> > > > >
> > > > >
> > > > > *Thanks,Kalyan Kumar Kalvagadda* | Software Engineer
> > > > > t. (469) 279- <00>5732
> > > > > cloudera.com <https://www.cloudera.com>
> > > > >
> > > > > [image: Cloudera] <https://www.cloudera.com/>
> > > > >
> > > > > [image: Cloudera on Twitter] <https://twitter.com/cloudera>
> [image:
> > > > > Cloudera on Facebook] <https://www.facebook.com/cloudera> [image:
> > > > Cloudera
> > > > > on LinkedIn] <https://www.linkedin.com/company/cloudera>
> > > > > --
> > > > >
> > > > > On Wed, Apr 25, 2018 at 6:58 PM, Alexander Kolbasov <
> > > ak...@cloudera.com>
> > > > > wrote:
> > > > >
> > > > > > Hive 3 introduced the notion of catalog. What do people think
> about
> > > > > > supporting it in Sentry as well? At some point we will move to
> > Hive-3
> > > > and
> > > > > > then it will become a requirement I guess.
> > > > > >
> > > > > > - Alex
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Sentry Release Schedule for FGP and ABAC

[Alexander Kolbasov]

One aspect we should consider is git history - working on feature branch
tends to produce a complicated git tree with many merges instead of the
linear tree that we get by working on master directly.

On Tue, May 1, 2018 at 8:57 AM, Na Li <lina...@cloudera.com> wrote:

> Steve,
>
> I am OK for ABAC to be on feature branch if you make sure 1) pull latest
> master to your feature branch 2) Let us review your changes with all unit
> tests pass.
>
> Thanks,
>
> Lina
>
> On Tue, May 1, 2018 at 10:48 AM, Stephen Moist <mo...@cloudera.com> wrote:
>
> > I’m fine with putting everything into a feature branch for now.  Right
> > now, the initial ABAC patch is a working solution.  It’s not the final
> > solution that we plan to deliver.  We could keep iterating on
> SENTRY-2201 <
> > https://issues.apache.org/jira/browse/SENTRY-2201> and adding more code
> > to a single patch and merge it back into master.  I don’t think anyone in
> > the community is going to want to review a 10mb patch.  So, I think going
> > forward, we will submit patches to an abac feature branch.  We plan to
> keep
> > iterating and expanding on the code base.  We want to always have a end
> to
> > end working solution at all time that our QA team can test.  Once the
> > Sentry community feels that abac is stable, we can merge it into master.
> > This way it 1) doesn’t impact Sentry 2.1 2) We don’t ship an incomplete
> > feature in 2.1 3)We can keep moving forward with development.
> >
> > With that said, I expect then the Sentry community (and more specifically
> > Committers) to stay on top of the changes we’re making to this feature
> > branch.  I don’t want everyone to ignore it for a few months and then
> start
> > re-reveiwing with it as we merge it back into master when we get to the
> end.
> >
> > Does this sound good to the community?
> >
> > > On Apr 30, 2018, at 6:35 PM, Alexander Kolbasov <ak...@cloudera.com>
> > wrote:
> > >
> > > Stephen,
> > >
> > > a lot depends on your plans in terms of breaking functionality. For
> > > example, one of the reasons Sentry HA was developed on a feature branch
> > was
> > > because it was a serious change in architecture and in broke
> > functionality
> > > for a while. I think some of the merge problems which Sergio referred
> to
> > > were caused by poor planning and communication - I think we are in much
> > > better shape now.
> > >
> > > One thing I would be concerned (in case you do your development in
> master
> > > branch) is that we end up shipping a release with half-baked feature
> > where
> > > there is a bunch of things that are there for the future but not really
> > > used. If you think this isn't a really a problem, developing on master
> is
> > > fine since it will automatically handle any potential conflicts with
> > > fine-grained privileges changes.
> > >
> > > Alex
> > >
> > > On Thu, Apr 26, 2018 at 9:08 AM, Stephen Moist <mo...@cloudera.com>
> > wrote:
> > >
> > >> Hey all, what does the current roadmap and release schedule look like
> > for
> > >> FGP and ABAC?  I’ve been told that FGP is going out in the next
> release,
> > >> ABAC is more slated for the summer.  How do we want to handle
> > simultaneous
> > >> development of these features?  For ABAC, our dev process is more
> agile.
> > >> So while we have a working version of ABAC right now in review, it’s
> not
> > >> the final solution.  We plan to iterate, improve, fix and add features
> > to
> > >> it over the next few months.  I had talked with Kalyan and Sergio
> > offline
> > >> once, they don’t like large patches and recommended not using a
> feature
> > >> branch.  I don’t see an issue with continuing to develop ABAC and FGP
> at
> > >> the same time and committing both to master.  We’ll add a switch in
> > ABAC to
> > >> turn the feature off for now through the next release.  What does the
> > >> community think about supporting development of two different features
> > at
> > >> once?
> >
> >
>

Re: Quick tips for code reviews

[Alexander Kolbasov]

+1

> On Apr 30, 2018, at 08:25, Sergio Pena  wrote:
> 
> Hey All,
> 
> I just want to share with the community about some helpful tips when
> submitting patches for review that are useful for reviewers:
> 
>   - Add a link to the review board on the JIRA related to the patch you're
>   working on.
>   - Add a link to the JIRA on the review board related to the patch you're
>   working on (Add the JIRA in the Bugs field).
> 
> These extra steps you do when submitting a patch save reviewers time when
> looking for references on the JIRA or the Review Board. For instance:
> 
>   - I miss the RB link to the patch. If I go to the JIRA, then I can find
>   it and click on the link to take me to the patch (1 click) instead of
>   searching the patch on the RB (takes time).
>   - I want to go to the JIRA that is referenced in the RB patch. If I go
>   to the Bugs field, then I can click on the link to take me to the JIRA (1
>   click) instead of opening another window, go to JIRA, edit the JIRA number
>   and press enter.
> 
> I encourage to use that on current and future patches to also have a
> reference to the review and who review the patch.
> 
> - Sergio

Re: questions on sentry

[Alexander Kolbasov]

To answer your second question - IP address based authorization isn't
supported.

- Alex

On Thu, Oct 19, 2017 at 8:20 AM Chen Song <chen.song...@gmail.com> wrote:

> Thanks Alexander.
>
> I asked because I see the HDFS ACLs sync feature for Sentry. I am clear
> now.
> Do you have any idea on my second question?
>
> Chen
>
> On Wed, Oct 18, 2017 at 10:59 PM Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
>> Hi Chen,
>>
>> Apache Sentry goal is to provide security features for other Apache
>> products - mainly Apache {Hive, Impala, Solr, Kafka}. It doesn't provide
>> HDFS access protections.
>>
>> - Alex.
>>
>> On Wed, Oct 18, 2017 at 12:05 PM, Chen Song <chen.song...@gmail.com>
>> wrote:
>>
>>> I have a few questions on Sentry. I have some thoughts in my mind but
>>> just
>>> want to confirm with the community.
>>>
>>> 1. Does Sentry have a way to block HDFS superuser from access HDFS
>>> directories?
>>> 2. Does Sentry support blacklisting a list of IPs from Hive or HDFS
>>> access?
>>>
>>> After a bit search, I don't find a way for both but I want to double
>>> check
>>> again.
>>>
>>> Chen
>>>
>>
>>

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4238 (patched)
<https://reviews.apache.org/r/65268/#comment275732>

The JIRA is about transaction retry logic and this mixes it with actual 
uses - it doesn't belong here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 246 (patched)
<https://reviews.apache.org/r/65268/#comment275733>

Since you are switching to time-based max it becomes the hard line. You 
want to either limit number of retries or total number but not both at the same 
time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 247 (original), 268 (patched)
<https://reviews.apache.org/r/65268/#comment275734>

This may make total sleep time greater then the allowed sleep time. You 
need to make sure that you never sleep longer then the remaining time left.


- Alexander Kolbasov


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Jan. 23, 2018, 11:40 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.
>  
> 
> 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/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/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/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

[DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Now that we have Sentry 2.0 release, I think it is a good time to step back
from fixing bugs and immediate problems and start discussions on roadmap
for Sentry going forward. Do we want to just keep it as is and improve
things here and there or we want to add new features?

What do people think?

- Alex

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Good point. There is some support for user-level privileges in 2.0 already
- do you think that it is not sufficient and is missing some parts?

Is there anyone reading this who participated in the user-level privileges
in Sentry work done earlier? Is there any design doc for this?

- Alex

On Thu, Jan 25, 2018 at 10:11 AM, Na Li <lina...@cloudera.com> wrote:

> Sasha,
>
> It would be nice to have more features for sentry.
>
> For example, make user-based privileges working. So user can assign user
> directly to a role instead of through group.
>
> Lina
>
> On Thu, Jan 25, 2018 at 11:58 AM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > Now that we have Sentry 2.0 release, I think it is a good time to step
> back
> > from fixing bugs and immediate problems and start discussions on roadmap
> > for Sentry going forward. Do we want to just keep it as is and improve
> > things here and there or we want to add new features?
> >
> > What do people think?
> >
> > - Alex
> >
>

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Thanks for the link - it is nice to integrate this discussion with JIRA
keywords. Looks like we need to go through the list and add categorize it
into short-term and long-term buckets.

I think Sergio's idea of doing smaller releases with small number of
features included makes sense.  We can vote for individual features, of
course but it only makes sense if someone actually commits to implementing
it.

Looks like so far the discussion is about improving user-level privileges -
it would be a good content for 2.1 release.

Is there some kind of design doc for user-level privileges in general? If
not, would it make sense to create one?

- Alex

On Thu, Jan 25, 2018 at 11:13 AM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> There is a section on the Wiki about roadmap ideas and JIRAs already
> created:
> https://cwiki.apache.org/confluence/display/SENTRY/
> Sentry+Roadmap+and+ideas
>
> I'm interested in having user-level privileges and special user privileges
> for objects owners.
>
> I got this from the linked above:
>   SENTRY-1073 User who creates a table should be granted all privileges on
> it by default
>   SENTRY-1068 Allow user who created a table to have "with grant" over that
> table by default
>   Creator of a table should have ownership of it (all privileges)
>   Allow privileges to be granted to users directly
>
> We should start planning the next Sentry 2.1 release based on the desired
> features. What about
> having 2 or 3 features on Sentry 2.1?
>
> I vote for:
> - user-level privileges (currently grant user to role is only supported)
> - default user privileges for objects owners
>
> Should we start a vote for new features for 2.1?
>
> - Sergio
>
> On Thu, Jan 25, 2018 at 12:46 PM, Kalyan Kumar Kalvagadda <
> kkal...@cloudera.com> wrote:
>
> > I would like to add something here.
> >
> >
> >1. Current support for user-based-privileges allows admin to grant a
> >role to user. Ideally, user-based-privileges feature should be
> allowing
> >administrator to grant privileges to individual users directly.
> >   -  I'm working on this to come up with a scope doc.
> >   2. Currently sentry stores only grant privileges. This is not
> >flexible. Let's say an administrator wants to grant role with select
> on
> > the
> >all tables in a database except for couple to them, he needs to
> > individual
> >select privileges for each table.
> >   1. Implementation should let you add a grant privilege on database
> >   and revokes privileges on the tables with in that database,
> >   2. This needs new look into privilege model that sentry currently
> > has.
> >
> >
> > -Kalyan
> >
> >
> > -Kalyan
> >
> > On Thu, Jan 25, 2018 at 12:16 PM, Alexander Kolbasov <ak...@cloudera.com
> >
> > wrote:
> >
> > > Good point. There is some support for user-level privileges in 2.0
> > already
> > > - do you think that it is not sufficient and is missing some parts?
> > >
> > > Is there anyone reading this who participated in the user-level
> > privileges
> > > in Sentry work done earlier? Is there any design doc for this?
> > >
> > > - Alex
> > >
> > > On Thu, Jan 25, 2018 at 10:11 AM, Na Li <lina...@cloudera.com> wrote:
> > >
> > > > Sasha,
> > > >
> > > > It would be nice to have more features for sentry.
> > > >
> > > > For example, make user-based privileges working. So user can assign
> > user
> > > > directly to a role instead of through group.
> > > >
> > > > Lina
> > > >
> > > > On Thu, Jan 25, 2018 at 11:58 AM, Alexander Kolbasov <
> > ak...@cloudera.com
> > > >
> > > > wrote:
> > > >
> > > > > Now that we have Sentry 2.0 release, I think it is a good time to
> > step
> > > > back
> > > > > from fixing bugs and immediate problems and start discussions on
> > > roadmap
> > > > > for Sentry going forward. Do we want to just keep it as is and
> > improve
> > > > > things here and there or we want to add new features?
> > > > >
> > > > > What do people think?
> > > > >
> > > > > - Alex
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Lina - would it make sense to create uber-JIRA for ULP, mark it with
"roadmap" keyword and start putting some of these as subtasks?

On Thu, Jan 25, 2018 at 11:35 AM, Na Li <lina...@cloudera.com> wrote:

> Sasha,
>
> The current user-based privilege missed some items:
>
>
>- Sentry policy has two service API: SentryPolicyService
>and SentryGenericPolicyService. The current implementation does not
> support
>user-based privilege for SentryGenericPolicyService
>- SENTRY-2091: User-based Privilege is broken by SENTRY-769. The patch
>is available for review.
>- Name Node need change to generate ACL using user privilege.
>   - The full snapshot update only contains authorization to roles
>   mapping and role to group mapping. *Need to add role to user mapping
>   in* SentryStore.retrieveFullRoleImageCore
>   - The delta updates are taken from table SENTRY_PERM_CHANGE, which
>   does not distinguish group based permission or user based
> permission. No
>   change is needed
>   - The user changes to a role is not included when sending delta
>   update from Sentry to NN. *Need to add AddUsers and DropUsers
>   in TRoleChanges*.
>   - Sentry only create ACL for group with ACL type
>   as AclEntryType.GROUP. *Need to add code to create ACL with type as *
>   AclEntryType.USER
>   - SentryINodeAttributesProvider.checkPermission
>  -> FSPermissionChecker.checkPermission
>  -> SentryINodeAttributesProvider.getAclFeature
>  -> SentryAuthorizationInfo.getAclEntries
>  -> SentryPermissions.constructAclEntry
>   - SentryStore.grantOptionCheck() has to be changed to find user level
>privilege.
>
> Thanks,
>
> Lina
>
> On Thu, Jan 25, 2018 at 1:13 PM, Sergio Pena <sergio.p...@cloudera.com>
> wrote:
>
> > There is a section on the Wiki about roadmap ideas and JIRAs already
> > created:
> > https://cwiki.apache.org/confluence/display/SENTRY/
> > Sentry+Roadmap+and+ideas
> >
> > I'm interested in having user-level privileges and special user
> privileges
> > for objects owners.
> >
> > I got this from the linked above:
> >   SENTRY-1073 User who creates a table should be granted all privileges
> on
> > it by default
> >   SENTRY-1068 Allow user who created a table to have "with grant" over
> that
> > table by default
> >   Creator of a table should have ownership of it (all privileges)
> >   Allow privileges to be granted to users directly
> >
> > We should start planning the next Sentry 2.1 release based on the desired
> > features. What about
> > having 2 or 3 features on Sentry 2.1?
> >
> > I vote for:
> > - user-level privileges (currently grant user to role is only supported)
> > - default user privileges for objects owners
> >
> > Should we start a vote for new features for 2.1?
> >
> > - Sergio
> >
> > On Thu, Jan 25, 2018 at 12:46 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > I would like to add something here.
> > >
> > >
> > >1. Current support for user-based-privileges allows admin to grant a
> > >role to user. Ideally, user-based-privileges feature should be
> > allowing
> > >administrator to grant privileges to individual users directly.
> > >   -  I'm working on this to come up with a scope doc.
> > >   2. Currently sentry stores only grant privileges. This is not
> > >flexible. Let's say an administrator wants to grant role with select
> > on
> > > the
> > >    all tables in a database except for couple to them, he needs to
> > > individual
> > >select privileges for each table.
> > >   1. Implementation should let you add a grant privilege on
> database
> > >   and revokes privileges on the tables with in that database,
> > >   2. This needs new look into privilege model that sentry currently
> > > has.
> > >
> > >
> > > -Kalyan
> > >
> > >
> > > -Kalyan
> > >
> > > On Thu, Jan 25, 2018 at 12:16 PM, Alexander Kolbasov <
> ak...@cloudera.com
> > >
> > > wrote:
> > >
> > > > Good point. There is some support for user-level privileges in 2.0
> > > already
> > > > - do you think that it is not sufficient and is missing some parts?
> > > >
> > > > Is there anyone reading this who participated in the user-level
> > > privileges
> > > &g

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Looks like we have a consensus of doing user-level privileges improvements
for 2.1. Let's see whether anyone wants to add more content.

On Thu, Jan 25, 2018 at 11:38 AM, Na Li <lina...@cloudera.com> wrote:

> Sasha,
>
> I have looked into how to complete the user-based privilege for a while,
> and can commit to implement it. I can work with Kalyan to create a design
> doc for user-based privilege.
>
> Thanks,
>
> Lina
>
> On Thu, Jan 25, 2018 at 1:35 PM, Na Li <lina...@cloudera.com> wrote:
>
> > Sasha,
> >
> > The current user-based privilege missed some items:
> >
> >
> >- Sentry policy has two service API: SentryPolicyService and
> SentryGenericPolicyService.
> >The current implementation does not support user-based privilege for
> >SentryGenericPolicyService
> >- SENTRY-2091: User-based Privilege is broken by SENTRY-769. The patch
> >is available for review.
> >- Name Node need change to generate ACL using user privilege.
> >   - The full snapshot update only contains authorization to roles
> >   mapping and role to group mapping. *Need to add role to user
> >   mapping in* SentryStore.retrieveFullRoleImageCore
> >   - The delta updates are taken from table SENTRY_PERM_CHANGE, which
> >   does not distinguish group based permission or user based
> permission. No
> >   change is needed
> >   - The user changes to a role is not included when sending delta
> >   update from Sentry to NN. *Need to add AddUsers and DropUsers
> >   in TRoleChanges*.
> >   - Sentry only create ACL for group with ACL type
> >   as AclEntryType.GROUP. *Need to add code to create ACL with type
> >   as *AclEntryType.USER
> >   - SentryINodeAttributesProvider.checkPermission
> >  -> FSPermissionChecker.checkPermission ->
> >  SentryINodeAttributesProvider.getAclFeature
> >  -> SentryAuthorizationInfo.getAclEntries -> SentryPermissions.
> >  constructAclEntry
> >   - SentryStore.grantOptionCheck() has to be changed to find user
> >level privilege.
> >
> > Thanks,
> >
> > Lina
> >
> > On Thu, Jan 25, 2018 at 1:13 PM, Sergio Pena <sergio.p...@cloudera.com>
> > wrote:
> >
> >> There is a section on the Wiki about roadmap ideas and JIRAs already
> >> created:
> >> https://cwiki.apache.org/confluence/display/SENTRY/Sentry+
> >> Roadmap+and+ideas
> >>
> >> I'm interested in having user-level privileges and special user
> privileges
> >> for objects owners.
> >>
> >> I got this from the linked above:
> >>   SENTRY-1073 User who creates a table should be granted all privileges
> on
> >> it by default
> >>   SENTRY-1068 Allow user who created a table to have "with grant" over
> >> that
> >> table by default
> >>   Creator of a table should have ownership of it (all privileges)
> >>   Allow privileges to be granted to users directly
> >>
> >> We should start planning the next Sentry 2.1 release based on the
> desired
> >> features. What about
> >> having 2 or 3 features on Sentry 2.1?
> >>
> >> I vote for:
> >> - user-level privileges (currently grant user to role is only supported)
> >> - default user privileges for objects owners
> >>
> >> Should we start a vote for new features for 2.1?
> >>
> >> - Sergio
> >>
> >> On Thu, Jan 25, 2018 at 12:46 PM, Kalyan Kumar Kalvagadda <
> >> kkal...@cloudera.com> wrote:
> >>
> >> > I would like to add something here.
> >> >
> >> >
> >> >1. Current support for user-based-privileges allows admin to grant
> a
> >> >role to user. Ideally, user-based-privileges feature should be
> >> allowing
> >> >administrator to grant privileges to individual users directly.
> >> >   -  I'm working on this to come up with a scope doc.
> >> >   2. Currently sentry stores only grant privileges. This is not
> >> >flexible. Let's say an administrator wants to grant role with
> select
> >> on
> >> > the
> >> >all tables in a database except for couple to them, he needs to
> >> > individual
> >> >select privileges for each table.
> >> >   1. Implementation should let you add a grant privilege on
> database
> >> >   and revokes privileges on the tables with in tha

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Agreed, making 2.1 with just user-level privileges improvements (plus set
of accumulated bug fixes) sounds reasonable.

On Thu, Jan 25, 2018 at 11:41 AM, Alexander Kolbasov <ak...@cloudera.com>
wrote:

> Looks like we have a consensus of doing user-level privileges improvements
> for 2.1. Let's see whether anyone wants to add more content.
>
> On Thu, Jan 25, 2018 at 11:38 AM, Na Li <lina...@cloudera.com> wrote:
>
>> Sasha,
>>
>> I have looked into how to complete the user-based privilege for a while,
>> and can commit to implement it. I can work with Kalyan to create a design
>> doc for user-based privilege.
>>
>> Thanks,
>>
>> Lina
>>
>> On Thu, Jan 25, 2018 at 1:35 PM, Na Li <lina...@cloudera.com> wrote:
>>
>> > Sasha,
>> >
>> > The current user-based privilege missed some items:
>> >
>> >
>> >- Sentry policy has two service API: SentryPolicyService and
>> SentryGenericPolicyService.
>> >The current implementation does not support user-based privilege for
>> >SentryGenericPolicyService
>> >- SENTRY-2091: User-based Privilege is broken by SENTRY-769. The
>> patch
>> >is available for review.
>> >- Name Node need change to generate ACL using user privilege.
>> >   - The full snapshot update only contains authorization to roles
>> >   mapping and role to group mapping. *Need to add role to user
>> >   mapping in* SentryStore.retrieveFullRoleImageCore
>> >   - The delta updates are taken from table SENTRY_PERM_CHANGE, which
>> >   does not distinguish group based permission or user based
>> permission. No
>> >   change is needed
>> >   - The user changes to a role is not included when sending delta
>> >   update from Sentry to NN. *Need to add AddUsers and DropUsers
>> >   in TRoleChanges*.
>> >   - Sentry only create ACL for group with ACL type
>> >   as AclEntryType.GROUP. *Need to add code to create ACL with type
>> >   as *AclEntryType.USER
>> >   - SentryINodeAttributesProvider.checkPermission
>> >  -> FSPermissionChecker.checkPermission ->
>> >  SentryINodeAttributesProvider.getAclFeature
>> >  -> SentryAuthorizationInfo.getAclEntries -> SentryPermissions.
>> >  constructAclEntry
>> >   - SentryStore.grantOptionCheck() has to be changed to find user
>> >level privilege.
>> >
>> > Thanks,
>> >
>> > Lina
>> >
>> > On Thu, Jan 25, 2018 at 1:13 PM, Sergio Pena <sergio.p...@cloudera.com>
>> > wrote:
>> >
>> >> There is a section on the Wiki about roadmap ideas and JIRAs already
>> >> created:
>> >> https://cwiki.apache.org/confluence/display/SENTRY/Sentry+
>> >> Roadmap+and+ideas
>> >>
>> >> I'm interested in having user-level privileges and special user
>> privileges
>> >> for objects owners.
>> >>
>> >> I got this from the linked above:
>> >>   SENTRY-1073 User who creates a table should be granted all
>> privileges on
>> >> it by default
>> >>   SENTRY-1068 Allow user who created a table to have "with grant" over
>> >> that
>> >> table by default
>> >>   Creator of a table should have ownership of it (all privileges)
>> >>   Allow privileges to be granted to users directly
>> >>
>> >> We should start planning the next Sentry 2.1 release based on the
>> desired
>> >> features. What about
>> >> having 2 or 3 features on Sentry 2.1?
>> >>
>> >> I vote for:
>> >> - user-level privileges (currently grant user to role is only
>> supported)
>> >> - default user privileges for objects owners
>> >>
>> >> Should we start a vote for new features for 2.1?
>> >>
>> >> - Sergio
>> >>
>> >> On Thu, Jan 25, 2018 at 12:46 PM, Kalyan Kumar Kalvagadda <
>> >> kkal...@cloudera.com> wrote:
>> >>
>> >> > I would like to add something here.
>> >> >
>> >> >
>> >> >1. Current support for user-based-privileges allows admin to
>> grant a
>> >> >role to user. Ideally, user-based-privileges feature should be
>> >> allowing
>> >> >administrator to grant privileges to individual users directly.
>> >&

Re: [DISCUSS] Sentry roadmap after 2.0

[Alexander Kolbasov]

Stephen - can you formulate these in JIRAs so we can add these to the
roadmap?

On Thu, Jan 25, 2018 at 12:31 PM, Stephen Moist <mo...@cloudera.com> wrote:

> A few things come to mind.
>
> Improving and expanding on the capabilities of the Sentry CLI.  It would
> be good to see all the other services integrate with Sentry in a consistent
> way.  Along with be able to administer grants/roles/etc through a common
> framework rather than say beeline.
>
> Improving documentation of Sentry’s integration, preferably with more
> examples of how to configure services.
>
> Adding access control on database operations such as drop table, insert,
> delete from, update, etc.
>
> I know for sure a feature we need is going to be tag based attribute
> control for Hive.
>
> These last two ideas would need some reworking to make Sentry more
> flexible to support these, and I’m willing to lead up the latter for tags.
>
> > On Jan 25, 2018, at 2:19 PM, Na Li <lina...@cloudera.com> wrote:
> >
> > https://issues.apache.org/jira/browse/SENTRY-2129 is create to track the
> > development activities for user-based privilege. I will add more
> sub-tasks
> > to it
> >
> > On Thu, Jan 25, 2018 at 1:42 PM, Alexander Kolbasov <ak...@cloudera.com>
> > wrote:
> >
> >> Agreed, making 2.1 with just user-level privileges improvements (plus
> set
> >> of accumulated bug fixes) sounds reasonable.
> >>
> >> On Thu, Jan 25, 2018 at 11:41 AM, Alexander Kolbasov <
> ak...@cloudera.com>
> >> wrote:
> >>
> >>> Looks like we have a consensus of doing user-level privileges
> >> improvements
> >>> for 2.1. Let's see whether anyone wants to add more content.
> >>>
> >>> On Thu, Jan 25, 2018 at 11:38 AM, Na Li <lina...@cloudera.com> wrote:
> >>>
> >>>> Sasha,
> >>>>
> >>>> I have looked into how to complete the user-based privilege for a
> while,
> >>>> and can commit to implement it. I can work with Kalyan to create a
> >> design
> >>>> doc for user-based privilege.
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Lina
> >>>>
> >>>> On Thu, Jan 25, 2018 at 1:35 PM, Na Li <lina...@cloudera.com> wrote:
> >>>>
> >>>>> Sasha,
> >>>>>
> >>>>> The current user-based privilege missed some items:
> >>>>>
> >>>>>
> >>>>>   - Sentry policy has two service API: SentryPolicyService and
> >>>> SentryGenericPolicyService.
> >>>>>   The current implementation does not support user-based privilege
> >> for
> >>>>>   SentryGenericPolicyService
> >>>>>   - SENTRY-2091: User-based Privilege is broken by SENTRY-769. The
> >>>> patch
> >>>>>   is available for review.
> >>>>>   - Name Node need change to generate ACL using user privilege.
> >>>>>  - The full snapshot update only contains authorization to roles
> >>>>>  mapping and role to group mapping. *Need to add role to user
> >>>>>  mapping in* SentryStore.retrieveFullRoleImageCore
> >>>>>  - The delta updates are taken from table SENTRY_PERM_CHANGE,
> >> which
> >>>>>  does not distinguish group based permission or user based
> >>>> permission. No
> >>>>>  change is needed
> >>>>>  - The user changes to a role is not included when sending delta
> >>>>>  update from Sentry to NN. *Need to add AddUsers and DropUsers
> >>>>>  in TRoleChanges*.
> >>>>>  - Sentry only create ACL for group with ACL type
> >>>>>  as AclEntryType.GROUP. *Need to add code to create ACL with type
> >>>>>  as *AclEntryType.USER
> >>>>>  - SentryINodeAttributesProvider.checkPermission
> >>>>> -> FSPermissionChecker.checkPermission ->
> >>>>> SentryINodeAttributesProvider.getAclFeature
> >>>>> -> SentryAuthorizationInfo.getAclEntries ->
> >> SentryPermissions.
> >>>>> constructAclEntry
> >>>>>  - SentryStore.grantOptionCheck() has to be changed to find user
> >>>>>   level privilege.
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 246 (patched)
<https://reviews.apache.org/r/65268/#comment275813>

If you don't think that you can use time because we do not know how much 
time it may take to execute transaction, then the whole point of this JIRA is 
moot and we shouldn't fix it in the first place.


- Alexander Kolbasov


On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Jan. 23, 2018, 11:40 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.
>  
> 
> 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/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/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/TestSentryStore.java
>  b4100278392986c161625a366212c6fef66ec0a9 
> 
> 
> Diff: https://reviews.apache.org/r/65268/diff/3/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: How to run bin/run_sentry.sh

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

> Hi,
>Does anyone know how to run bin/run_sentry.sh? Thanks!
>
>
> Xinran
>

Re: How to run bin/run_sentry.sh

[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 <lina...@cloudera.com> 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 <ak...@cloudera.com>:
>> 
>>> 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
>>>> 
>>> 
>> 
>> 

Question about user-level privileges

[Alexander Kolbasov]

I am wondering what is the relationship between "users" as defined in
Sentry and users as defined in Unix or LDAP or Active Directory.

Should it be allowed to assign permissions to a user that doesn't exist?
Should there be any validation if users? Should these be treated together
or independently?

Also, there is discussion about adding permissions not to roles but to
users directly. How is it different from adding permissions not to roles
but to groups directly?

So far Sentry used role-based model - do we want to change it to
entity-based model?

- Alex

Re: How to run bin/run_sentry.sh

[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 <yuxinran8...@gmail.com>
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 <ak...@cloudera.com>:
>
>> 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 <lina...@cloudera.com> 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 <ak...@cloudera.com>:
>> >>
>> >>> 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
>> >>>>
>> >>>
>> >>
>> >>
>>
>
>

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

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

[Alexander Kolbasov]

sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3496 (patched)
<https://reviews.apache.org/r/65268/#comment276416>

    Please provide more information - how much longer did it take? Was it one 
nanosecond or 2 hours?


- Alexander Kolbasov


On Jan. 31, 2018, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Jan. 31, 2018, 12:30 a.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/5/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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 <yuxinran8...@gmail.com>
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 <ak...@cloudera.com>:
>
>> 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 <yuxinran8...@gmail.com
>> >
>> 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 <ak...@cloudera.com>:
>> >
>> >> 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 <lina...@cloudera.com> 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 <ak...@cloudera.com>:
>> >> >>
>> >> >>> 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: 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/#review196494
---




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Line 414 (original), 416 (patched)
<https://reviews.apache.org/r/65268/#comment276160>

This is unrelated change, please remove from the changeset.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 231 (original), 236 (patched)
<https://reviews.apache.org/r/65268/#comment276173>

Do you need both?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 242 (original)
<https://reviews.apache.org/r/65268/#comment276169>

It may still be useful to count number of retries and log it. I was often 
looking for broblems by looking at this log entry.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 249 (patched)
<https://reviews.apache.org/r/65268/#comment276171>

Do you need extra () here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 250 (patched)
<https://reviews.apache.org/r/65268/#comment276170>

Why bvreak - you may adjust sleepTime to be smaller to fit within allotted 
time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 261 (patched)
<https://reviews.apache.org/r/65268/#comment276174>

What if this condition is false? In this case you retry immediately without 
sleeping



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 93 (patched)
<https://reviews.apache.org/r/65268/#comment276172>

The comment doesn't match the actual behavior - this is the initial sleep 
time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 95 (patched)
<https://reviews.apache.org/r/65268/#comment276162>

The name is confusing. In is not a retry interval but an upper bound of 
execution for retries.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 96 (patched)
<https://reviews.apache.org/r/65268/#comment276164>

Please add time unit in the name.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 98 (patched)
<https://reviews.apache.org/r/65268/#comment276163>

Can you express this via Time units?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 102 (patched)
<https://reviews.apache.org/r/65268/#comment276165>

Please add time unit in the name.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 103 (patched)
<https://reviews.apache.org/r/65268/#comment276166>

How did you come up with 30 sec and 90 sec numbers?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2593 (patched)
<https://reviews.apache.org/r/65268/#comment276168>

Please add comment explaining what are you testing here. Looks like the 
name is not telling the truth - you are testing something complicated about 
notifications.

What you really need is a test that verifies that your failed transactions 
fail more or less within allotted time, but this test doesn't test for it.


- Alexander Kolbasov


On Jan. 29, 2018, 9:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Jan. 29, 2018, 9:34 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.
>

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 230 (original), 241 (patched)
<https://reviews.apache.org/r/65268/#comment276691>

Why do we need sleepTime and newSleepTime?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 243 (patched)
<https://reviews.apache.org/r/65268/#comment276692>

What is the purpose of this variable?
Instead, it would be useful to compute the final time right away.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 243 (original), 255 (patched)
<https://reviews.apache.org/r/65268/#comment276694>

Please use `//` style comments



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 261 (patched)
<https://reviews.apache.org/r/65268/#comment276693>

We don't want to break here, instead we want to reduce the sleep time to 
match the remaining balance.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 276 (patched)
<https://reviews.apache.org/r/65268/#comment276695>

I don't see why you need newSleepTime at all.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 281 (patched)
<https://reviews.apache.org/r/65268/#comment276696>

Do you need space after `giving up` ?


- Alexander Kolbasov


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

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 224 (original), 224 (patched)
<https://reviews.apache.org/r/65268/#comment276686>

1) The first senrence should describe shortly what the class is.
2) Please use javadoc (HTML) style formatting.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Line 225 (original), 225 (patched)
<https://reviews.apache.org/r/65268/#comment276688>

The first sentence does not explain what this class is doing.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 232 (patched)
<https://reviews.apache.org/r/65268/#comment276690>

Given exponential nature, how quickly do we usually reach this point?


- Alexander Kolbasov


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 
>   
> 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/6/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Please welcome Kalyan Kalvagadda to Apache PMC!

[Alexander Kolbasov]

Please welcome Kalyan Kalvagadda who is joining Apache Sentry PMC!

- Alex

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 262 (patched)
<https://reviews.apache.org/r/65268/#comment276710>

The problem with this is demonstrated by your test. When I run it, I see:

Retried for 283 milli seconds. Giving upInsert 

Your configuration has a limit of 500 ms but you only retried for 283 
milliseconds, you could easily try harder.


- Alexander Kolbasov


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

[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)
<https://reviews.apache.org/r/65268/#comment277273>

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)
<https://reviews.apache.org/r/65268/#comment277274>

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

Fwd: Travel Assistance applications open. Please inform your communities

[Alexander Kolbasov]

> Begin forwarded message:
> 
> From: Gavin McDonald 
> Subject: Travel Assistance applications open. Please inform your communities
> Date: February 14, 2018 at 1:34:11 AM PST
> To: travel-assista...@apache.org
> Reply-To: priv...@sentry.apache.org
> Reply-To: travel-assista...@apache.org
> 
> Hello PMCs.
> 
> Please could you forward on the below email to your dev and user lists.
> 
> Thanks
> 
> Gav…
> 
> —
> The Travel Assistance Committee (TAC) are pleased to announce that travel 
> assistance applications for ApacheCon NA 2018 are now open!
> 
> We will be supporting ApacheCon NA Montreal, Canada on 24th - 29th September 
> 2018
> 
>  TAC exists to help those that would like to attend ApacheCon events, but are 
> unable to do so for financial reasons. 
> For more info on this years applications and qualifying criteria, please 
> visit the TAC website at < http://www.apache.org/travel/ 
>  >. Applications are now open and will close 
> 1st May. 
> 
> Important: Applications close on May 1st, 2018. Applicants have until the 
> closing date above to submit their applications (which should contain as much 
> supporting material as required to efficiently and accurately process their 
> request), this will enable TAC to announce successful awards shortly 
> afterwards. 
> 
> As usual, TAC expects to deal with a range of applications from a diverse 
> range of backgrounds. We therefore encourage (as always) anyone thinking 
> about sending in an application to do so ASAP.   
> We look forward to greeting many of you in Montreal
> 
> Kind Regards, 
> Gavin - (On behalf of the Travel Assistance Committee)
> — 
> 
> 

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

[Alexander Kolbasov]

> On Feb. 18, 2018, 5:29 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Line 96 (original), 96 (patched)
> > <https://reviews.apache.org/r/65704/diff/1/?file=1962380#file1962380line96>
> >
> > This is Ok, but probably an overkill - we only need to sync table 
> > renames, so you may check that thist ALTER TABLE actually renames table (or 
> > database) and only sync in these cases.
> 
> Na Li wrote:
> For adding column, should we wait as well since privilege can be on 
> column? If we only wait for rename, we still have issues

Possibly - it would be useful to see what are things that can happen in ALTER 
TABLE and see which ones need synchronization and which don't. Synchronization 
is expensive, so it is better to avoid it unless really necessary.


- Alexander


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


On Feb. 18, 2018, 5:25 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 18, 2018, 5:25 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
Lines 106 (patched)
<https://reviews.apache.org/r/65268/#comment278191>

I would consider changing default here from 250ms to 125 ms. With the 
current sleep the first delay is closer to 400-500 ms which is a bit too much 
for initial sleep
with 125 ms default we get initial sleep close to 200 ms while not 
increasing retries a lot.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3432 (patched)
<https://reviews.apache.org/r/65268/#comment278192>

30 sec is too much for this test - it delays the whole test for 30 sec. Can 
you test it with the whole test taking ~2 seconds?


- Alexander Kolbasov


On Feb. 12, 2018, 11:50 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65268/
> ---
> 
> (Updated Feb. 12, 2018, 11:50 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/8/
> 
> 
> Testing
> ---
> 
> Made sure all the tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

[DISCUSS] Allowing Java 8 features in Sentry

[Alexander Kolbasov]

Hello Sentry hackers,

We are now compiling Sentry with Java 8. I would like to propose allowing
Java 8 features in Sentry code. As of now it is difficult to enforce strict
Java 7 compatibility, so we may just as well embrace the reality and start
using Java 8 which hopefully will improve the quality/readability of the
code.

Any thoughts on this?

- Alex

[DISCUSS] Adjustments to the commit process

[Alexander Kolbasov]

Hello everyone,

I would like to propose an adjustment to the commit process in Sentry
project. The idea is to require that commit should not be done by the
person providing the change but by some other committer. This committer's
responsibility is to ensure that all code review concerns were addressed in
one way or another and to do a final sanity check. This committer can be
one of the reviewers or someone who didn't review the code.

What do you think?

- Alex

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

[Alexander Kolbasov]

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



Does the test show the problem before the fix?


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 96 (original), 98 (patched)
<https://reviews.apache.org/r/65704/#comment278245>

Is there any benefit in creating separate function that is only used here? 
Why not just inline the code?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 146 (patched)
<https://reviews.apache.org/r/65704/#comment278246>

Please remove all this logging. This whole thing can be simplified to

`return !newDbName.equalsIgnoreCase(oldDbName) || 
!newTableName.equalsIgnoreCase(oldTableName);`

You don't need this if statement at all.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2412-2413 (patched)
<https://reviews.apache.org/r/65704/#comment278247>

What does this have to do with the JIRA that you are fixing? Looks like 
some kinnd of separate issue.


- Alexander Kolbasov


On Feb. 22, 2018, 6:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 22, 2018, 6:55 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  9b0aeb2 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/4/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 227 (patched)
<https://reviews.apache.org/r/65642/#comment277920>

It is better to use TimeUnit class for time conversions


- Alexander Kolbasov


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> sentry CreateTime is the difference, measured in milliseconds, between the 
> current time and midnight, January 1, 1970 UTC. hive granttime is in seconds. 
> So need to convert the time.
> 
> The original code just cost the timestamp from long to int without converting 
> milliseconds to seconds. Therefore, the timestamp value is wrong when 
> retrieving the privilege from hive.
> 
> The solution is to convert the time correctly.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  b536283 
> 
> 
> Diff: https://reviews.apache.org/r/65642/diff/1/
> 
> 
> Testing
> ---
> 
> tested manually by stepping into code. I cannot test it in unit test because 
> Hive returns timestamp as -1L for test mode
> 
>   DDLTask.writeGrantInfo
> static String writeGrantInfo(List privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> public int compare(HivePrivilegeInfo o1, HivePrivilegeInfo o2) {
>   int compare = o1.getObject().compareTo(o2.getObject());
>   if (compare == 0) {
> compare = o1.getPrincipal().compareTo(o2.getPrincipal());
>   }
> 
>   if (compare == 0) {
> compare = o1.getPrivilege().compareTo(o2.getPrivilege());
>   }
> 
>   return compare;
> }
>   });
>   Iterator var3 = privileges.iterator();
> 
>   while(var3.hasNext()) {
> HivePrivilegeInfo privilege = (HivePrivilegeInfo)var3.next();
> HivePrincipal principal = privilege.getPrincipal();
> HivePrivilegeObject resource = privilege.getObject();
> HivePrincipal grantor = privilege.getGrantorPrincipal();
> appendNonNull(builder, resource.getDbname(), true);
> appendNonNull(builder, resource.getObjectName());
> appendNonNull(builder, resource.getPartKeys());
> appendNonNull(builder, resource.getColumns());
> appendNonNull(builder, principal.getName());
> appendNonNull(builder, principal.getType());
> appendNonNull(builder, privilege.getPrivilege().getName());
> appendNonNull(builder, privilege.isGrantOption());
> appendNonNull(builder, testMode ? -1L : 
> (long)privilege.getGrantTime() * 1000L); <-- in test mode, does not 
> return real timestamp
> appendNonNull(builder, grantor.getName());
>   }
> 
>   return builder.toString();
> } else {
>   return "";
> }
>   }
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: [DISCUSS] Adjustments to the commit process

[Alexander Kolbasov]

For assigning committers I think this may be a simple informal request -
for example to one of the reviewers or to someone else to volunteer. It may
delay commits a bit indeed, but I don't think it will be a problem.

The problem I am trying to address is the quality of the review process.
Suppose we have some change C for which Alice have some comments and Bob
have some and eventually Alice says Ship it and it isn't clear whether Bob
is Ok with the change or not, but since ALice is the committer, the author
of the patch thinks that it is ok to submit it right away. That's where a
'sanity check' person would be useful.


On Thu, Feb 22, 2018 at 3:49 PM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> How is this committer going to be assigned?
> This might lead to some complications if the committer assigned leave for
> vacations afterward and the community is not notified. It will end up
> delaying the commits and the author (being a committer) won't be able to
> commit the patch due to this process. What are we trying to solve with
> this?
>
> Btw, I've seen in other projects that some committers usually wait 1 or 2
> days to commit a patch after a +1 has been done on it. This is to allow
> other reviewers to disagree with the +1 and give more feedback before
> committing the patch. Would this help?
>
> - Sergio
>
> On Thu, Feb 22, 2018 at 1:29 PM, Stephen Moist <mo...@cloudera.com> wrote:
>
> > Sounds reasonable to me as long as they can get someone to do the commit
> > in a reasonable timeframe.  I wouldn’t want to have to wait days for it
> to
> > get in after it has been properly reviewed.
> >
> > > On Feb 22, 2018, at 12:22 PM, Alexander Kolbasov <ak...@cloudera.com>
> > wrote:
> > >
> > > Hello everyone,
> > >
> > > I would like to propose an adjustment to the commit process in Sentry
> > > project. The idea is to require that commit should not be done by the
> > > person providing the change but by some other committer. This
> committer's
> > > responsibility is to ensure that all code review concerns were
> addressed
> > in
> > > one way or another and to do a final sanity check. This committer can
> be
> > > one of the reviewers or someone who didn't review the code.
> > >
> > > What do you think?
> > >
> > > - Alex
> >
> >
>

Re: Review Request 65704: SENTRY-2143: Table renames should synchronize with Sentry

[Alexander Kolbasov]

> On Feb. 22, 2018, 6:56 p.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 146 (patched)
> > <https://reviews.apache.org/r/65704/diff/3/?file=1963273#file1963273line146>
> >
> > Please remove all this logging. This whole thing can be simplified to
> > 
> > `return !newDbName.equalsIgnoreCase(oldDbName) || 
> > !newTableName.equalsIgnoreCase(oldTableName);`
> > 
> > You don't need this if statement at all.
> 
> Na Li wrote:
> Sergio and Kalyan want to check if the values to be null before the 
> comparision. They also want to log in warning level in case one of them are 
> null.

You don't need to because equalsIgnoreCase allow nulls.


- Alexander


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


On Feb. 22, 2018, 6:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65704/
> ---
> 
> (Updated Feb. 22, 2018, 6:55 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> wait for HMS sync at alter table, which including table rename and changing 
> table columns
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  24d7763 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a01 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  5fe6625 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  9b0aeb2 
> 
> 
> Diff: https://reviews.apache.org/r/65704/diff/4/
> 
> 
> Testing
> ---
> 
> unit tests succeeded
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles()

[Alexander Kolbasov]

> On Dec. 18, 2017, 6:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Line 267 (original), 271 (patched)
> > <https://reviews.apache.org/r/64661/diff/3/?file=1922322#file1922322line271>
> >
> > I think there is a way to directly test for membership in the JDOQL - 
> > can you check this?
> > 
> > We have similar code in SentryStore which does that.
> 
> Arjun Mishra wrote:
> Sasha not sure what exactly you are asking for? Are you asking for cases 
> when a role is not mapped to any group? Also are your comments specific to my 
> change? 
> 
> I did look at SentryStore. There are many places in the class where we 
> addRolesFilter and run executeWithMap but I didn't see any check for 
> membership being done here.

I mean something similar to this:

  private Set getRoleNamesForGroupsCore(PersistenceManager pm, 
Set groups) {
 Query query = pm.newQuery(MSentryGroup.class);
 query.setFilter(":p1.contains(this.groupName)");
 List sentryGroups = (List) 
query.execute(groups.toArray());


- Alexander


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


On Dec. 19, 2017, 8:04 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -------
> 
> (Updated Dec. 19, 2017, 8:04 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When Solr is using Sentry server for authorization, it issues a lot of calls 
> to {{getGroupsByRoles()}} function in {{DelegateSentryStore}}.
> 
> This function isn't very efficient - it walks over each role in the set, 
> obtains role by name, get groups for each role, and collects all group names 
> into a set.
> 
> It may be possible to optimize it.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  4cb46abc5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
>  69d16238f 
> 
> 
> Diff: https://reviews.apache.org/r/64661/diff/4/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: [VOTE] - Release Apache Sentry 1.7.1

[Alexander Kolbasov]

Thanks Colm!

Happy new year!
On Sun, Dec 24, 2017 at 05:31 Colm O hEigeartaigh <cohei...@apache.org>
wrote:

> With 3 binding +1 votes, and no other votes, this vote passes. I'll do the
> release.
>
> Colm.
>
> On Wed, Dec 20, 2017 at 5:18 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
>> +1
>>
>> On Wed, Dec 20, 2017 at 7:04 AM, Sergio Pena <sergio.p...@cloudera.com>
>> wrote:
>>
>>> Agree. I checked the rest of the release (checksums, gpg, build) and it
>>> is
>>> good.
>>>
>>> +1
>>>
>>> On Wed, Dec 20, 2017 at 2:51 AM Colm O hEigeartaigh <cohei...@apache.org
>>> >
>>> wrote:
>>>
>>> > Yeah I saw that afterwards + updated the branch. I didn't think it was
>>> > worthy of cancelling the vote though. What do others think?
>>> >
>>> > Colm.
>>> >
>>> > On Wed, Dec 20, 2017 at 12:29 AM, Sergio Pena <
>>> sergio.p...@cloudera.com>
>>> > wrote:
>>> >
>>> >> Colm, the CHANGELOG does not have the 1.7.1 fixes.
>>> >> Do you need to add those?
>>> >>
>>> >> Sergio
>>> >>
>>> >> On Mon, Dec 18, 2017 at 8:58 AM, Colm O hEigeartaigh <
>>> cohei...@apache.org
>>> >> > wrote:
>>> >>
>>> >>> This is a vote to release Apache Sentry 1.7.1.
>>> >>>
>>> >>> Artifacts:
>>> >>>
>>> >>> https://dist.apache.org/repos/dist/dev/sentry/1.7.1/
>>> >>>
>>> >>> Git tag:
>>> >>>
>>> >>> https://github.com/apache/sentry/tree/release-1.7.1
>>> >>>
>>> >>> Issues fixed:
>>> >>>
>>> >>> https://issues.apache.org/jira/projects/SENTRY/versions/12342308
>>> >>>
>>> >>> +1 from me.
>>> >>>
>>> >>> Colm.
>>> >>>
>>> >>> --
>>> >>> Colm O hEigeartaigh
>>> >>>
>>> >>> Talend Community Coder
>>> >>> http://coders.talend.com
>>> >>>
>>> >>
>>> >>
>>> >
>>> >
>>> > --
>>> > Colm O hEigeartaigh
>>> >
>>> > Talend Community Coder
>>> > http://coders.talend.com
>>> >
>>>
>>
>>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles()

[Alexander Kolbasov]

> On Dec. 18, 2017, 6:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Line 267 (original), 271 (patched)
> > <https://reviews.apache.org/r/64661/diff/3/?file=1922322#file1922322line271>
> >
> > I think there is a way to directly test for membership in the JDOQL - 
> > can you check this?
> > 
> > We have similar code in SentryStore which does that.
> 
> Arjun Mishra wrote:
> Sasha not sure what exactly you are asking for? Are you asking for cases 
> when a role is not mapped to any group? Also are your comments specific to my 
> change? 
> 
> I did look at SentryStore. There are many places in the class where we 
> addRolesFilter and run executeWithMap but I didn't see any check for 
> membership being done here.
> 
> Alexander Kolbasov wrote:
> I mean something similar to this:
> 
>   private Set getRoleNamesForGroupsCore(PersistenceManager 
> pm, Set groups) {
>  Query query = pm.newQuery(MSentryGroup.class);
>  query.setFilter(":p1.contains(this.groupName)");
>  List sentryGroups = (List) 
> query.execute(groups.toArray());
> 
> Arjun Mishra wrote:
> I did add addRolesFilter check. See below. And addRolesFilter does the 
> contains check. So something similar to the above is already being done. Yes?
> 
> *
> QueryParamBuilder paramBuilder = QueryParamBuilder.addRolesFilter(query, 
> null, roles);
> 
> 
> *
> public static QueryParamBuilder addRolesFilter(Query query, 
> QueryParamBuilder paramBuilder,
>  Set roleNames) {
> query.declareVariables(MSentryRole.class.getName() + " role");
> if (paramBuilder == null) {
>   paramBuilder = new QueryParamBuilder();
> }
> if (roleNames == null || roleNames.isEmpty()) {
>   return paramBuilder;
> }
> paramBuilder.newChild().addSet("role.roleName == ", roleNames);
> paramBuilder.addString("roles.contains(role)");
> return paramBuilder;
>   }
> 
> Arjun Mishra wrote:
> Sasha, I get the below exception when not adding the map link. So it 
> looks like we need to explitly addRolesFilter and execute with map
> 
> 
> ==
> javax.jdo.JDOUserException: Query has reference to member "roleName" of 
> class "org.apache.sentry.provider.db.service.model.MSentryGroup" yet this 
> doesnt exist!
>   at 
> org.apache.sentry.provider.db.generic.service.persistent.TestDelegateSentryStore.testGetGroupsByRoleNames(TestDelegateSentryStore.java:160)
> Caused by: org.datanucleus.exceptions.NucleusUserException: Query has 
> reference to member "roleName" of class 
> "org.apache.sentry.provider.db.service.model.MSentryGroup" yet this doesnt 
> exist!
>   at 
> org.apache.sentry.provider.db.generic.service.persistent.TestDelegateSentryStore.testGetGroupsByRoleNames(TestDelegateSentryStore.java:160)
> 
> 
> ==

MSentryGroup doesn't have roleName, it has roles instead. I am not sure that 
you can do similar check here.

But looking at the caller:

Set roleNames = 
store.getRolesByGroups(request.getComponent(), groups);
Set tSentryRoles = Sets.newHashSet();
for (String roleName : roleNames) {
  Set groupsForRoleName = 
store.getGroupsByRoles(request.getComponent(), Sets.newHashSet(roleName));
  tSentryRoles.add(new TSentryRole(roleName, groupsForRoleName));
}

So first it gets all roles belonging to given set of groups. Then it walks all 
roles and for each role it gets groups belonging to this role. This is a weird 
request, but putting this aside, would it make sense to use a single 
transaction to answer this question?
Otherwise, if group belongs to N roles, we need to use N transactions to get 
this


- Alexander


---
This is an automatically generated e-mail. To reply

Re: Review Request 68488: SENTRY-2367: Implement subsystem to allow for pluggable attribute providers and transports

[Alexander Kolbasov]

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




sentry-spi/src/main/java/org/apache/sentry/spi/DefaultProviderLoader.java
Lines 27 (patched)
<https://reviews.apache.org/r/68488/#comment291380>

What is this file - where does it come from?



sentry-spi/src/main/java/org/apache/sentry/spi/Provider.java
Lines 23 (patched)
<https://reviews.apache.org/r/68488/#comment291381>

Where does this file come from?



sentry-spi/src/main/java/org/apache/sentry/spi/ProviderFactory.java
Lines 25 (patched)
<https://reviews.apache.org/r/68488/#comment291382>

where ddoes this file come from?



sentry-spi/src/main/java/org/apache/sentry/spi/ProviderManager.java
Lines 34 (patched)
<https://reviews.apache.org/r/68488/#comment291383>

Typo? "Provider manager *what*"



sentry-spi/src/main/java/org/apache/sentry/spi/Spi.java
Lines 24 (patched)
<https://reviews.apache.org/r/68488/#comment291384>

Is this file coming from somewhere?


- Alexander Kolbasov


On Aug. 24, 2018, 1:15 a.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68488/
> ---
> 
> (Updated Aug. 24, 2018, 1:15 a.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2367
> https://issues.apache.org/jira/browse/SENTRY-2367
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Basic implementation of the Java SPI. Abstracted to provide Factory 
> configuration for
> instances of Service defined.
> 
> Currently stand alone module.  Will be built used by MDCM in future commits.
> 
> 
> Diffs
> -
> 
>   lombok.config PRE-CREATION 
>   pom.xml dcf107680c3b395819043136bcc6e179a60ada35 
>   sentry-spi/pom.xml PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/DefaultProviderLoader.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/Provider.java PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderFactory.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderLoader.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderManager.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/Spi.java PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/package-info.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProvider.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderFactory.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplA.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplB.java 
> PRE-CREATION 
>   
> sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplNotLoaded.java
>  PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestSpi.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/TestProviderManager.java 
> PRE-CREATION 
>   
> sentry-spi/src/test/resources/META-INF/services/org.apache.sentry.spi.SomeTestProviderFactory
>  PRE-CREATION 
>   sentry-spi/src/test/resources/META-INF/services/org.apache.sentry.spi.Spi 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68488/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 68488: SENTRY-2367: Implement subsystem to allow for pluggable attribute providers and transports

[Alexander Kolbasov]

> On Aug. 29, 2018, 8:29 p.m., Steve Moist wrote:
> > lombok.config
> > Lines 1 (patched)
> > 
> >
> > In introducing lombok into sentry.  Will the configs for the existing 
> > sl4j need to be updated or modified?
> > 
> > I would recommend that we move this lombok change into a seperate 
> > changeset.  I would want all of Sentry to be consistant with it's logging 
> > mechanism.

Agreed - it is always better not to mix two independent changes together.


- Alexander


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


On Aug. 24, 2018, 1:15 a.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68488/
> ---
> 
> (Updated Aug. 24, 2018, 1:15 a.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2367
> https://issues.apache.org/jira/browse/SENTRY-2367
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Basic implementation of the Java SPI. Abstracted to provide Factory 
> configuration for
> instances of Service defined.
> 
> Currently stand alone module.  Will be built used by MDCM in future commits.
> 
> 
> Diffs
> -
> 
>   lombok.config PRE-CREATION 
>   pom.xml dcf107680c3b395819043136bcc6e179a60ada35 
>   sentry-spi/pom.xml PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/DefaultProviderLoader.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/Provider.java PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderFactory.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderLoader.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/ProviderManager.java 
> PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/Spi.java PRE-CREATION 
>   sentry-spi/src/main/java/org/apache/sentry/spi/package-info.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProvider.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderFactory.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplA.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplB.java 
> PRE-CREATION 
>   
> sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestProviderImplNotLoaded.java
>  PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/SomeTestSpi.java 
> PRE-CREATION 
>   sentry-spi/src/test/java/org/apache/sentry/spi/TestProviderManager.java 
> PRE-CREATION 
>   
> sentry-spi/src/test/resources/META-INF/services/org.apache.sentry.spi.SomeTestProviderFactory
>  PRE-CREATION 
>   sentry-spi/src/test/resources/META-INF/services/org.apache.sentry.spi.Spi 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68488/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Arjun Mishra - New Apache Sentry Committer

[Alexander Kolbasov]

It is my pleasure to introduce a new Apache Sentry committer who is very
familiar to people on this list - Arjun Mishra! Welcome Arjun!

- Alex

Re: Review Request 67846: SENTRY-2283 Multiple versions of metrics on the classpath causes Sentry to not startup

[Alexander Kolbasov]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
Line 20 (original), 20 (patched)
<https://reviews.apache.org/r/67846/#comment288761>

Shouldn't this be done automatically by the shading plugin?


- Alexander Kolbasov


On July 6, 2018, 7:06 p.m., Steve Moist wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67846/
> ---
> 
> (Updated July 6, 2018, 7:06 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Excluding conflicting dependencies and shading io.dropwizard.metrics.
> 
> 
> Diffs
> -
> 
>   pom.xml 488426593 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 5f8a5afb4 
>   sentry-binding/sentry-binding-kafka/pom.xml 239eeba5f 
>   sentry-dist/src/license/THIRD-PARTY.properties b39e1b6ca 
>   sentry-dist/src/main/assembly/bin.xml 72773df1e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  81c614a34 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  3532ef33d 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  8d6713acd 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  b87d29040 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  0cd405b54 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  3bf300bef 
>   sentry-provider/sentry-provider-db/pom.xml a8a15bfb1 
>   sentry-service/sentry-service-server/pom.xml a103c1e25 
>   sentry-tests/sentry-tests-solr/pom.xml db33ee9f4 
>   sentry-tests/sentry-tests-sqoop/pom.xml e280c9eb5 
> 
> 
> Diff: https://reviews.apache.org/r/67846/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Moist
> 
>