You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by Na Li <li...@cloudera.com> on 2017/04/05 22:29:31 UTC

Review Request 58221: SENTRY-1649 move HMS follower to runServer

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

Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 


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


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

> On April 6, 2017, 8:07 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 310 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line325>
> >
> >     what happens if the submitted task has not been shut down and someone calls startHMSFollower? Is it safe to allow so?

I think so. While old HMSFollwer executor is shutting down (including the old HMSFollwer), the start will cause the new HMSFollwer executor and new HMSFollwer to be created and the task scheduled. They are seperate instances. 

This scenario is reasonable. For example, user may stop and then start before shutting down finishes. We either allow start before shutting down finishes or make start do nothing before shutting down finishes.

In stop(), "status = Status.NOT_STARTED;" is called before stopping HMSFollwer, which allows start() to call runServer() and then start HMSFollwer. So the current code does allow this scenario.

We can test this case.

- Na

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

On April 6, 2017, 10:40 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 10:40 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Hao Hao <ha...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 310 (patched)
<https://reviews.apache.org/r/58221/#comment244140>

    what happens if the submitted task has not been shut down and someone calls startHMSFollower? Is it safe to allow so?


- Hao Hao


On April 5, 2017, 10:29 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 5, 2017, 10:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 6, 2017, 5:48 p.m., Lei Xu wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 289 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line304>
> >
> >     According to the code style guide, the opening brace should be at the same line with the function. 
> >     
> >     ```
> >     privated void startHMSFollower(Configuration conf) {
> >     ...
> >     ```

will fix. Thanks!


> On April 6, 2017, 5:48 p.m., Lei Xu wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 301 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line316>
> >
> >     Please also log the exception.

will fix.


> On April 6, 2017, 5:48 p.m., Lei Xu wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 307 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line322>
> >
> >     Please put open brace to the same line as the function signature. 
> >     
> >     Also there should be a space between `if` and parenthesis.
> >     
> >     Please remove the blank line at line 313.

will fix


- Na


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


On April 6, 2017, 10:40 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 10:40 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Lei Xu <le...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 289 (patched)
<https://reviews.apache.org/r/58221/#comment244109>

    According to the code style guide, the opening brace should be at the same line with the function. 
    
    ```
    privated void startHMSFollower(Configuration conf) {
    ...
    ```



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 301 (patched)
<https://reviews.apache.org/r/58221/#comment244110>

    Please also log the exception.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 307 (patched)
<https://reviews.apache.org/r/58221/#comment244111>

    Please put open brace to the same line as the function signature. 
    
    Also there should be a space between `if` and parenthesis.
    
    Please remove the blank line at line 313.


- Lei Xu


On April 5, 2017, 3:29 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 5, 2017, 3:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 6, 2017, 7:04 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 310 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line325>
> >
> >     Why not directly shutdown and make it null?
> 
> Na Li wrote:
>     I checked the behavior of shutdown(): "Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted. Invocation has no additional effect if already shut down." It may take some time before shutdown() returns and could throw "SecurityException". 
>     
>     It is safer to set the hmsFollowerExecutor null before calling the shutdown(). If someone calls startHMSFollower() during this time, it would work properly, i.e., it can be started and HmsFollower work can be scheduled.
> 
> Na Li wrote:
>     I changed it. Now, the HMSFollower is directly shut down and set to null

I have updated the code to reuse the HMSFollower instance


- Na


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


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 6, 2017, 7:04 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 310 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line325>
> >
> >     Why not directly shutdown and make it null?

I checked the behavior of shutdown(): "Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted. Invocation has no additional effect if already shut down." It may take some time before shutdown() returns and could throw "SecurityException". 

It is safer to set the hmsFollowerExecutor null before calling the shutdown(). If someone calls startHMSFollower() during this time, it would work properly, i.e., it can be started and HmsFollower work can be scheduled.


> On April 6, 2017, 7:04 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 313 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line328>
> >
> >     Extra line?

Will remove the extra line


- Na


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


On April 5, 2017, 10:29 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 5, 2017, 10:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 6, 2017, 7:04 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 310 (patched)
> > <https://reviews.apache.org/r/58221/diff/1/?file=1685541#file1685541line325>
> >
> >     Why not directly shutdown and make it null?
> 
> Na Li wrote:
>     I checked the behavior of shutdown(): "Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted. Invocation has no additional effect if already shut down." It may take some time before shutdown() returns and could throw "SecurityException". 
>     
>     It is safer to set the hmsFollowerExecutor null before calling the shutdown(). If someone calls startHMSFollower() during this time, it would work properly, i.e., it can be started and HmsFollower work can be scheduled.

I changed it. Now, the HMSFollower is directly shut down and set to null


- Na


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


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Hao Hao <ha...@cloudera.com>.

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


Fix it, then Ship it!





sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 310 (patched)
<https://reviews.apache.org/r/58221/#comment244117>

    Why not directly shutdown and make it null?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 313 (patched)
<https://reviews.apache.org/r/58221/#comment244116>

    Extra line?


- Hao Hao


On April 5, 2017, 10:29 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 5, 2017, 10:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> >     Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
>     It is possible. I will add "synchronized" in front of it.
> 
> Alexander Kolbasov wrote:
>     Under what circumstances is it possible? If it is possible, it may not be sufficient to add synchronized() - you may need to protect access to the variables as well, but I a not convinced that it is possible. Do we run our tests in parallel?
> 
> Na Li wrote:
>     I am not familar with how Sentry service is actually used. Hypathetically, it is possible that several threads hold the same instance and call its close() concurrently.
> 
> Alexander Kolbasov wrote:
>     This should never hapen during regular use - I think this may only happen if tests are running in parallel but I guess that it will break many things.

close() is removed. So this issue does not apply any more.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> >     I think this should never happen.
> 
> Na Li wrote:
>     From API, it could be thrown
> 
> Alexander Kolbasov wrote:
>     from the API - yes, but this shouldn't happen in our case, so we should log something about the fact that this should never happen.
> 
> Na Li wrote:
>     Will do.

add the message in log


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line145>
> >
> >     Why do you need this?
> 
> Na Li wrote:
>     to track the task that schedules hmsFollower, so I can cancel it in stop()

it is removed.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160>
> >
> >     Why do you need this?
> 
> Na Li wrote:
>     to track the task that schedules sentryStoreCleaner, so I can cancel it in stop()

I still keep it to mark that the cleaner is scheduled if the future is not null.

The problems are
1. We need to create sentryStoreCleanService and schedule cleaner (call startSentryStoreCleaner()) in runServer() if SentryService.start() is called after SentryService.stop(), which shutdown sentryStoreCleanService. Otherwise, the cleaner cannot be scheduled any more. Test may fail.
2. We need to create sentryStoreCleanService and schedule cleaner in SentryService constructor (call startSentryStoreCleaner()), so the test TestDbSentryOnFailureHookLoading won't fail. It expects the context to be not null in createContext() before calling SentryService.start(). 
3. To satify item 1. and 2., we need to call startSentryStoreCleaner() at both constructor and runServer(). To avoid scheduling the cleaner twice, we need to detect if it is scheduled already.
4. When I tried to fix test failures, it seems it is better to create the sentryStoreCleanService at the beginning of creating SentryService. Then checking if sentryStoreCleanService is null or not cannot detect if the cleaner is scheduled already.


When the SentryService is created, we want to create sentryStoreCleanService before calling startSentryStoreCleaner(). But after stop(), we need to create sentryStoreCleanService within startSentryStoreCleaner. So I choose to use sentryStoreCleanerFuture to decide if we should schedule the cleaner


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> >     This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.
> 
> Alexander Kolbasov wrote:
>     It seems to be not exactly wrong, but makes things more complicated then they should be. You need to shutdown the executor on exit anyway, so why cancel scheduling using futures?
> 
> Na Li wrote:
>     Based on java doc and discussion in http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice, it seems future.cancel can stop the task from being scheduled in the future. I can run the testing code and verify the behavior of Future.cancel() tomorrow. If this is the case, then cancel future is less overhead then shutting down the ExecutorService. Once ExecutorService is shutdown, it is useless, and we have to create a new ExecutorService in order to use it.
> 
> Alexander Kolbasov wrote:
>     Cancel is useful if we want to stop execution but want to keep the executor for later use. Normally (non-test use) we do want to cancel the executor itself - we never need to reuse the executor. For tests we can keep it, but then we need to have test-only code, and the overhead isn't important.
> 
> Na Li wrote:
>     I will discuss with you tomorrow at hipchat to see if I will change back to shutdown the executor at SentryService.stop()

I have changed the code and shutdown the executor at SentryService.stop()


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160>
> >
> >     Why do you need this?
> 
> Na Li wrote:
>     to track the task that schedules sentryStoreCleaner, so I can cancel it in stop()
> 
> Na Li wrote:
>     I still keep it to mark that the cleaner is scheduled if the future is not null.
>     
>     The problems are
>     1. We need to create sentryStoreCleanService and schedule cleaner (call startSentryStoreCleaner()) in runServer() if SentryService.start() is called after SentryService.stop(), which shutdown sentryStoreCleanService. Otherwise, the cleaner cannot be scheduled any more. Test may fail.
>     2. We need to create sentryStoreCleanService and schedule cleaner in SentryService constructor (call startSentryStoreCleaner()), so the test TestDbSentryOnFailureHookLoading won't fail. It expects the context to be not null in createContext() before calling SentryService.start(). 
>     3. To satify item 1. and 2., we need to call startSentryStoreCleaner() at both constructor and runServer(). To avoid scheduling the cleaner twice, we need to detect if it is scheduled already.
>     4. When I tried to fix test failures, it seems it is better to create the sentryStoreCleanService at the beginning of creating SentryService. Then checking if sentryStoreCleanService is null or not cannot detect if the cleaner is scheduled already.
>     
>     
>     When the SentryService is created, we want to create sentryStoreCleanService before calling startSentryStoreCleaner(). But after stop(), we need to create sentryStoreCleanService within startSentryStoreCleaner. So I choose to use sentryStoreCleanerFuture to decide if we should schedule the cleaner

Thanks for the explanation. This looks like complete mess. You don't have to untangle it in this JIRA, but please

1) Add a comment with the above explanation
2) File a follow-up JIRA for cleaning this mess up.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> >     This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.
> 
> Alexander Kolbasov wrote:
>     It seems to be not exactly wrong, but makes things more complicated then they should be. You need to shutdown the executor on exit anyway, so why cancel scheduling using futures?
> 
> Na Li wrote:
>     Based on java doc and discussion in http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice, it seems future.cancel can stop the task from being scheduled in the future. I can run the testing code and verify the behavior of Future.cancel() tomorrow. If this is the case, then cancel future is less overhead then shutting down the ExecutorService. Once ExecutorService is shutdown, it is useless, and we have to create a new ExecutorService in order to use it.

Cancel is useful if we want to stop execution but want to keep the executor for later use. Normally (non-test use) we do want to cancel the executor itself - we never need to reuse the executor. For tests we can keep it, but then we need to have test-only code, and the overhead isn't important.


- Alexander


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


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 5:38 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> >     Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
>     It is possible. I will add "synchronized" in front of it.
> 
> Alexander Kolbasov wrote:
>     Under what circumstances is it possible? If it is possible, it may not be sufficient to add synchronized() - you may need to protect access to the variables as well, but I a not convinced that it is possible. Do we run our tests in parallel?

I am not familar with how Sentry service is actually used. Hypathetically, it is possible that several threads hold the same instance and call its close() concurrently.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> >     I think this should never happen.
> 
> Na Li wrote:
>     From API, it could be thrown
> 
> Alexander Kolbasov wrote:
>     from the API - yes, but this shouldn't happen in our case, so we should log something about the fact that this should never happen.

Will do.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> >     This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.
> 
> Alexander Kolbasov wrote:
>     It seems to be not exactly wrong, but makes things more complicated then they should be. You need to shutdown the executor on exit anyway, so why cancel scheduling using futures?
> 
> Na Li wrote:
>     Based on java doc and discussion in http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice, it seems future.cancel can stop the task from being scheduled in the future. I can run the testing code and verify the behavior of Future.cancel() tomorrow. If this is the case, then cancel future is less overhead then shutting down the ExecutorService. Once ExecutorService is shutdown, it is useless, and we have to create a new ExecutorService in order to use it.
> 
> Alexander Kolbasov wrote:
>     Cancel is useful if we want to stop execution but want to keep the executor for later use. Normally (non-test use) we do want to cancel the executor itself - we never need to reuse the executor. For tests we can keep it, but then we need to have test-only code, and the overhead isn't important.

I will discuss with you tomorrow at hipchat to see if I will change back to shutdown the executor at SentryService.stop()


- Na


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


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 5:38 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> >     Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
>     It is possible. I will add "synchronized" in front of it.

Under what circumstances is it possible? If it is possible, it may not be sufficient to add synchronized() - you may need to protect access to the variables as well, but I a not convinced that it is possible. Do we run our tests in parallel?


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> >     I think this should never happen.
> 
> Na Li wrote:
>     From API, it could be thrown

from the API - yes, but this shouldn't happen in our case, so we should log something about the fact that this should never happen.


- Alexander


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


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 5:38 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> >     Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
>     It is possible. I will add "synchronized" in front of it.
> 
> Alexander Kolbasov wrote:
>     Under what circumstances is it possible? If it is possible, it may not be sufficient to add synchronized() - you may need to protect access to the variables as well, but I a not convinced that it is possible. Do we run our tests in parallel?
> 
> Na Li wrote:
>     I am not familar with how Sentry service is actually used. Hypathetically, it is possible that several threads hold the same instance and call its close() concurrently.

This should never hapen during regular use - I think this may only happen if tests are running in parallel but I guess that it will break many things.


- Alexander


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


On April 13, 2017, 4:47 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 13, 2017, 4:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> >     This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.

It seems to be not exactly wrong, but makes things more complicated then they should be. You need to shutdown the executor on exit anyway, so why cancel scheduling using futures?


- Alexander


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


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 108 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689529#file1689529line114>
> >
> >     No need to return at the end of the function.

will remove


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 376 (original), 383 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689529#file1689529line389>
> >
> >     As you are fixing this, can you fix this line as well - we know that dbNam == null so we shouldn't try to convert it to string.

will update


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 77 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line77>
> >
> >     Style - it is better to have field declarations first, followed by methods. The close() method is usually somewhere closer to the end but we don't have any specific policy on that.

I will move it to the end of the file


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> >     Do you ever expect concurrent calls to close() or not?

It is possible. I will add "synchronized" in front of it.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 81 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line81>
> >
> >     Note that you need to stop the executor first. Otherwise while you are calling close() it may run HMSFOllower which isn't good.

stop() should be called before this, so the task is already cancelled. therefore, the executor won't schedule the task again. To be safe, I will change the order.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 92 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line92>
> >
> >     Can you call awaitTermination() before you called shutdown()?
> >     
> >     The doc says:
> >     
> >     Blocks until all tasks have completed execution after a shutdown request, or the timeout occurs, or the current thread is interrupted, whichever happens first.

I believe we should call shutdown() first. So no new tasks can be scheduled


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> >     I think this should never happen.

From API, it could be thrown


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 99 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line99>
> >
> >     Should we call shutdownNow() in this case?

Yes. I will add it


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line145>
> >
> >     Why do you need this?

to track the task that schedules hmsFollower, so I can cancel it in stop()


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160>
> >
> >     Why do you need this?

to track the task that schedules sentryStoreCleaner, so I can cancel it in stop()


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> >     This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.
> 
> Alexander Kolbasov wrote:
>     It seems to be not exactly wrong, but makes things more complicated then they should be. You need to shutdown the executor on exit anyway, so why cancel scheduling using futures?

Based on java doc and discussion in http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice, it seems future.cancel can stop the task from being scheduled in the future. I can run the testing code and verify the behavior of Future.cancel() tomorrow. If this is the case, then cancel future is less overhead then shutting down the ExecutorService. Once ExecutorService is shutdown, it is useless, and we have to create a new ExecutorService in order to use it.


- Na


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


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160>
> >
> >     Why do you need this?
> 
> Na Li wrote:
>     to track the task that schedules sentryStoreCleaner, so I can cancel it in stop()
> 
> Na Li wrote:
>     I still keep it to mark that the cleaner is scheduled if the future is not null.
>     
>     The problems are
>     1. We need to create sentryStoreCleanService and schedule cleaner (call startSentryStoreCleaner()) in runServer() if SentryService.start() is called after SentryService.stop(), which shutdown sentryStoreCleanService. Otherwise, the cleaner cannot be scheduled any more. Test may fail.
>     2. We need to create sentryStoreCleanService and schedule cleaner in SentryService constructor (call startSentryStoreCleaner()), so the test TestDbSentryOnFailureHookLoading won't fail. It expects the context to be not null in createContext() before calling SentryService.start(). 
>     3. To satify item 1. and 2., we need to call startSentryStoreCleaner() at both constructor and runServer(). To avoid scheduling the cleaner twice, we need to detect if it is scheduled already.
>     4. When I tried to fix test failures, it seems it is better to create the sentryStoreCleanService at the beginning of creating SentryService. Then checking if sentryStoreCleanService is null or not cannot detect if the cleaner is scheduled already.
>     
>     
>     When the SentryService is created, we want to create sentryStoreCleanService before calling startSentryStoreCleaner(). But after stop(), we need to create sentryStoreCleanService within startSentryStoreCleaner. So I choose to use sentryStoreCleanerFuture to decide if we should schedule the cleaner
> 
> Alexander Kolbasov wrote:
>     Thanks for the explanation. This looks like complete mess. You don't have to untangle it in this JIRA, but please
>     
>     1) Add a comment with the above explanation
>     2) File a follow-up JIRA for cleaning this mess up.

I removed the future and only start cleaner at runServer(). The last two builds are green. So I don't need it any more.


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




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

    No need to return at the end of the function.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 376 (original), 383 (patched)
<https://reviews.apache.org/r/58221/#comment244663>

    As you are fixing this, can you fix this line as well - we know that dbNam == null so we shouldn't try to convert it to string.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 77 (patched)
<https://reviews.apache.org/r/58221/#comment244666>

    Style - it is better to have field declarations first, followed by methods. The close() method is usually somewhere closer to the end but we don't have any specific policy on that.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 78 (patched)
<https://reviews.apache.org/r/58221/#comment244665>

    Do you ever expect concurrent calls to close() or not?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 81 (patched)
<https://reviews.apache.org/r/58221/#comment244664>

    Note that you need to stop the executor first. Otherwise while you are calling close() it may run HMSFOllower which isn't good.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 92 (patched)
<https://reviews.apache.org/r/58221/#comment244667>

    Can you call awaitTermination() before you called shutdown()?
    
    The doc says:
    
    Blocks until all tasks have completed execution after a shutdown request, or the timeout occurs, or the current thread is interrupted, whichever happens first.



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

    I think this should never happen.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 99 (patched)
<https://reviews.apache.org/r/58221/#comment244670>

    Should we call shutdownNow() in this case?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 145 (patched)
<https://reviews.apache.org/r/58221/#comment244673>

    Why do you need this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 160 (patched)
<https://reviews.apache.org/r/58221/#comment244674>

    Why do you need this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 350 (patched)
<https://reviews.apache.org/r/58221/#comment244675>

    This is most likely wrong. What does the cancel of the future do? What task does it actually cancel given that this is periodic executor? U think you should just use shutdown on the executor instead of this.


- Alexander Kolbasov


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 370 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line398>
> >
> >     Do we need to call awaitTermination() after shutdownNow()? This is existing code, but still.
> 
> Na Li wrote:
>     I don't know the reason why

I talked to Kalyan. He mentioned that it is realted to database update. We want to let it end gracefully.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 425 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454>
> >
> >     The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed.
> 
> Na Li wrote:
>     the synchronized is orignal. You mentioned if multiple threads accese it at the same time, since we don't protect variables, it is still not thread safe.

do you want me to remove the comment?


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 196 (original), 186 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line198>
> >
> >     Can you clarify why it should be started in constructor and why would tests fail otherwise?
> 
> Na Li wrote:
>     the test TestDbSentryOnFailureHookLoading setup calls AbstractTestWithDbProvider.createContext(), which expects the context to be not null after creating SentryService and before calling SentryService.start(). If we don't schedule the cleaner in constructor, we will fail at assumeNotNull(context); if we schedul the cleaner in constructor, it does not fail at assumeNotNull(context); 
>     
>     AbstractTestWithDbProvider.createContext()
>     {
>     ...
>         assumeNotNull(context);
>         context = AbstractTestWithHiveServer.createContext(properties);
>         policyFile
>             .setUserGroupMapping(StaticUserGroup.getStaticMapping())
>             .write(context.getPolicyFile(), policyFilePath);
>     
>         startSentryService();
>         ...
>     }

That's rather weird - the context is created right after you verify that it is not null. How can that work? And how does the cleaner service affects this strange assert?


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 368 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line396>
> >
> >     10 seconds seems a bit excessive
> 
> Na Li wrote:
>     This is original valued. What value you suggest?
> 
> Na Li wrote:
>     Can we address it in another issue when we understand the reason why it is such large number? My experience is that when changing code, it could break at some seemingly unreleated area.

Sure, let's not address it here.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 425 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454>
> >
> >     The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed.
> 
> Na Li wrote:
>     the synchronized is orignal. You mentioned if multiple threads accese it at the same time, since we don't protect variables, it is still not thread safe.
> 
> Na Li wrote:
>     do you want me to remove the comment?

It may be better to update it - the current comment contradicts the code.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > There are many unrelated formatting changes which should be removed.

I will fix it in next update


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 107 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line107>
> >
> >     Why do you need the future for the cleaner?

I explained in the same question above. This is duplicate.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 196 (original), 186 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line198>
> >
> >     Can you clarify why it should be started in constructor and why would tests fail otherwise?

the test TestDbSentryOnFailureHookLoading setup calls AbstractTestWithDbProvider.createContext(), which expects the context to be not null after creating SentryService and before calling SentryService.start(). If we don't schedule the cleaner in constructor, we will fail at assumeNotNull(context); if we schedul the cleaner in constructor, it does not fail at assumeNotNull(context); 

AbstractTestWithDbProvider.createContext()
{
...
    assumeNotNull(context);
    context = AbstractTestWithHiveServer.createContext(properties);
    policyFile
        .setUserGroupMapping(StaticUserGroup.getStaticMapping())
        .write(context.getPolicyFile(), policyFilePath);

    startSentryService();
    ...
}


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 221 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line249>
> >
> >     Why do we need to start it again if it is already started in constructor?

In test, if stop() is called and then start() is called, not calling startSentryStoreCleaner() in runServer() would fail the test


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 290 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line318>
> >
> >     Exception is too broad here - you know what are possible exceptions. In the older code you were also handling exceptions from HMSFollower construction - now you are not.
> >     
> >     It is impossible to handle the failure so the best thing to do here is to propagate the exception.

I will change it to IllegalArgumentException. It is the only possible exception


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 298 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line326>
> >
> >     Style. In such cases it is better to use
> >     
> >     if (hmsFollowerExecutor == null) {
> >       return; // nothing to stop
> >     }
> >     
> >     This way you don't need to have long chain of conditions and follow the else clauses.

will update


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 300 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line328>
> >
> >     Your try { } block is too broad. Are you trying shutdown() or shutdownNow()? Also you run shutdownNow inside a try block but then call it in the catch anyway, so it isn't very clear what are you guarding.
> >     
> >     It may be better to surround specific calls with try blocks so that you can handle individual failures properly

I will wrap shutdownNode in catch block with try-catch


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 305 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line333>
> >
> >     Why are you using SENTRY_HMSFOLLOWER_INTERVAL_MILLS as a timeout value here? Not that it is particularly wrong, but at least it is worth a comment.

I will add a comment


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 327 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line355>
> >
> >     It would be nice to log the fact that we started the service (if we did)

will add log


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 330 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line358>
> >
> >     I don't think you need a future here, but even if you do it is better to check it outside the try block and return immediately if it is not null.

I do need to check future. Will update accordingly to style


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 333 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line361>
> >
> >     Do we need to start an executor if it isn't going to be used due to SENTRY_STORE_CLEAN_PERIOD_SECONDS settings?

I will start the executor only when it is needed


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 356 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line384>
> >
> >     Exception is too broad here

will change it to IllegalArgumentException


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 364 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line392>
> >
> >     Same comment - it is usually better to have
> >     
> >     if (sentryStoreCleanerService == null) {
> >       return;
> >     }

will update


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 367 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line395>
> >
> >     shutdownNow() should be good enough here.

This is original code I moved from constructor. There may be some reason I don't know that the shut down is so complicated.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 368 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line396>
> >
> >     10 seconds seems a bit excessive

This is original valued. What value you suggest?


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 370 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line398>
> >
> >     Do we need to call awaitTermination() after shutdownNow()? This is existing code, but still.

I don't know the reason why


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 374 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line402>
> >
> >     catch parameter ie is unused

I will add log


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 425 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454>
> >
> >     The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed.

the synchronized is orignal. You mentioned if multiple threads accese it at the same time, since we don't protect variables, it is still not thread safe.


- Na


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


On April 13, 2017, 7:56 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 13, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 196 (original), 186 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line198>
> >
> >     Can you clarify why it should be started in constructor and why would tests fail otherwise?
> 
> Na Li wrote:
>     the test TestDbSentryOnFailureHookLoading setup calls AbstractTestWithDbProvider.createContext(), which expects the context to be not null after creating SentryService and before calling SentryService.start(). If we don't schedule the cleaner in constructor, we will fail at assumeNotNull(context); if we schedul the cleaner in constructor, it does not fail at assumeNotNull(context); 
>     
>     AbstractTestWithDbProvider.createContext()
>     {
>     ...
>         assumeNotNull(context);
>         context = AbstractTestWithHiveServer.createContext(properties);
>         policyFile
>             .setUserGroupMapping(StaticUserGroup.getStaticMapping())
>             .write(context.getPolicyFile(), policyFilePath);
>     
>         startSentryService();
>         ...
>     }
> 
> Alexander Kolbasov wrote:
>     That's rather weird - the context is created right after you verify that it is not null. How can that work? And how does the cleaner service affects this strange assert?

Now, it is not started at constructor. So the issue is removed.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 425 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454>
> >
> >     The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed.
> 
> Na Li wrote:
>     the synchronized is orignal. You mentioned if multiple threads accese it at the same time, since we don't protect variables, it is still not thread safe.
> 
> Na Li wrote:
>     do you want me to remove the comment?
> 
> Alexander Kolbasov wrote:
>     It may be better to update it - the current comment contradicts the code.

I have removed the comment that it is not thread safe


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 368 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line396>
> >
> >     10 seconds seems a bit excessive
> 
> Na Li wrote:
>     This is original valued. What value you suggest?

Can we address it in another issue when we understand the reason why it is such large number? My experience is that when changing code, it could break at some seemingly unreleated area.


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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



There are many unrelated formatting changes which should be removed.


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 107 (patched)
<https://reviews.apache.org/r/58221/#comment244985>

    Why do you need the future for the cleaner?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 196 (original), 186 (patched)
<https://reviews.apache.org/r/58221/#comment244986>

    Can you clarify why it should be started in constructor and why would tests fail otherwise?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 221 (patched)
<https://reviews.apache.org/r/58221/#comment244987>

    Why do we need to start it again if it is already started in constructor?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 290 (patched)
<https://reviews.apache.org/r/58221/#comment244988>

    Exception is too broad here - you know what are possible exceptions. In the older code you were also handling exceptions from HMSFollower construction - now you are not.
    
    It is impossible to handle the failure so the best thing to do here is to propagate the exception.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 298 (patched)
<https://reviews.apache.org/r/58221/#comment244989>

    Style. In such cases it is better to use
    
    if (hmsFollowerExecutor == null) {
      return; // nothing to stop
    }
    
    This way you don't need to have long chain of conditions and follow the else clauses.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 300 (patched)
<https://reviews.apache.org/r/58221/#comment244991>

    Your try { } block is too broad. Are you trying shutdown() or shutdownNow()? Also you run shutdownNow inside a try block but then call it in the catch anyway, so it isn't very clear what are you guarding.
    
    It may be better to surround specific calls with try blocks so that you can handle individual failures properly



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 305 (patched)
<https://reviews.apache.org/r/58221/#comment244990>

    Why are you using SENTRY_HMSFOLLOWER_INTERVAL_MILLS as a timeout value here? Not that it is particularly wrong, but at least it is worth a comment.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 327 (patched)
<https://reviews.apache.org/r/58221/#comment245002>

    It would be nice to log the fact that we started the service (if we did)



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 330 (patched)
<https://reviews.apache.org/r/58221/#comment244995>

    I don't think you need a future here, but even if you do it is better to check it outside the try block and return immediately if it is not null.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 333 (patched)
<https://reviews.apache.org/r/58221/#comment244992>

    Do we need to start an executor if it isn't going to be used due to SENTRY_STORE_CLEAN_PERIOD_SECONDS settings?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 356 (patched)
<https://reviews.apache.org/r/58221/#comment244993>

    Exception is too broad here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 364 (patched)
<https://reviews.apache.org/r/58221/#comment244994>

    Same comment - it is usually better to have
    
    if (sentryStoreCleanerService == null) {
      return;
    }



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 367 (patched)
<https://reviews.apache.org/r/58221/#comment244996>

    shutdownNow() should be good enough here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 368 (patched)
<https://reviews.apache.org/r/58221/#comment244997>

    10 seconds seems a bit excessive



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 370 (patched)
<https://reviews.apache.org/r/58221/#comment244998>

    Do we need to call awaitTermination() after shutdownNow()? This is existing code, but still.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 374 (patched)
<https://reviews.apache.org/r/58221/#comment245001>

    catch parameter ie is unused



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 425 (patched)
<https://reviews.apache.org/r/58221/#comment245000>

    The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed.


- Alexander Kolbasov


On April 13, 2017, 7:56 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 13, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 18, 2017, 9:41 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 314 (patched)
> > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line346>
> >
> >     Nit: this line seems too long. Similar to above ones longer than 100.

I will make it shorter. Sasha wants to mention that exception is impossible to occure.


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 18, 2017, 9:41 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 314 (patched)
> > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line346>
> >
> >     Nit: this line seems too long. Similar to above ones longer than 100.
> 
> Na Li wrote:
>     I will make it shorter. Sasha wants to mention that exception is impossible to occure.

now the message is around 80.


- Na


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Hao Hao <ha...@cloudera.com>.

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


Fix it, then Ship it!




Overall the changes looks good to me. Thanks a lot Lina for picking this up!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 314 (patched)
<https://reviews.apache.org/r/58221/#comment245376>

    Nit: this line seems too long. Similar to above ones longer than 100.


- Hao Hao


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 19, 2017, 9:08 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 274 (patched)
> > <https://reviews.apache.org/r/58221/diff/17/?file=1694265#file1694265line306>
> >
> >     It seems that there is a valid use case for tests to call start() with diferent configurations to start new HMSFollower in start() method - this way, for example, tests may start it with different metastore URI. This contradicts my earlier comment, but I didn't know about this use case.
> >     
> >     So I would consider managing HMSFollower here as well.

will do.


- Na


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 19, 2017, 9:08 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 274 (patched)
> > <https://reviews.apache.org/r/58221/diff/17/?file=1694265#file1694265line306>
> >
> >     It seems that there is a valid use case for tests to call start() with diferent configurations to start new HMSFollower in start() method - this way, for example, tests may start it with different metastore URI. This contradicts my earlier comment, but I didn't know about this use case.
> >     
> >     So I would consider managing HMSFollower here as well.
> 
> Na Li wrote:
>     will do.

HMSFollower is create in runServer() now.


- Na


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


On April 25, 2017, 9:15 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 9:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/22/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 274 (patched)
<https://reviews.apache.org/r/58221/#comment245508>

    It seems that there is a valid use case for tests to call start() with diferent configurations to start new HMSFollower in start() method - this way, for example, tests may start it with different metastore URI. This contradicts my earlier comment, but I didn't know about this use case.
    
    So I would consider managing HMSFollower here as well.


- Alexander Kolbasov


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 20, 2017, 12:17 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 20, 2017, 12:17 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/18/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated May 5, 2017, 3:47 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 


Diff: https://reviews.apache.org/r/58221/diff/25/

Changes: https://reviews.apache.org/r/58221/diff/24-25/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Line 172 (original), 172 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708101#file1708101line172>
> >
> >     Understand that this delay is causing issues during tests but making it 0 may not be a best choice.
> >     
> >     It may take some time for the server to initialize the database and be completely up to handle the notifications. 
> >     
> >     If 60000 milli sec is higher value, we should have some smaller value instead.
> 
> Na Li wrote:
>     since we prevent connecting to local meta store, HMSFollower does nothing if Hive meta store is not up. It does not hurt to have this value being zero, and allow sentry getting updates sooner.

"sentry-1753 Make HMSFollower initial delay configurable" is created


- Na


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Line 172 (original), 172 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708101#file1708101line172>
> >
> >     Understand that this delay is causing issues during tests but making it 0 may not be a best choice.
> >     
> >     It may take some time for the server to initialize the database and be completely up to handle the notifications. 
> >     
> >     If 60000 milli sec is higher value, we should have some smaller value instead.
> 
> Na Li wrote:
>     since we prevent connecting to local meta store, HMSFollower does nothing if Hive meta store is not up. It does not hurt to have this value being zero, and allow sentry getting updates sooner.
> 
> Na Li wrote:
>     "sentry-1753 Make HMSFollower initial delay configurable" is created

The initial delay was coming out of the blue and isn't clear why it is needed at all. What kind of server initialization are you talking about?


- Alexander


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 131 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708098#file1708098line133>
> >
> >     Can you update the comment explanining why this check was needed? I think you are using the HiveConf.ConfVars.METASTOREURIS.varname to make sure that the Hive server is already UP while running unit tests.

Yes. We want to prevent HMSFollower from using local meta store.


> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
> > Lines 182 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708100#file1708100line182>
> >
> >     As part of shutdownAndAwaitTermination, shutdown is called first to stop new tasks to getting scheduled. 
> >     
> >     Later awaitTermination is called with timeout value while actually the interval at which the tasks are scheduled giving it enough time to terminate before calling shutdownNow.
> >     
> >     When awaitTermination is called for the second time to log an error, i feel the timeout can be 0, as we need not give any more time for the task to temrinate.

If HMSFollower is getting full snapshot, it may take longer. So wait for additional time is useful.


> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Line 172 (original), 172 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708101#file1708101line172>
> >
> >     Understand that this delay is causing issues during tests but making it 0 may not be a best choice.
> >     
> >     It may take some time for the server to initialize the database and be completely up to handle the notifications. 
> >     
> >     If 60000 milli sec is higher value, we should have some smaller value instead.

since we prevent connecting to local meta store, HMSFollower does nothing if Hive meta store is not up. It does not hurt to have this value being zero, and allow sentry getting updates sooner.


- Na


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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




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

    Can you update the comment explanining why this check was needed? I think you are using the HiveConf.ConfVars.METASTOREURIS.varname to make sure that the Hive server is already UP while running unit tests.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 182 (patched)
<https://reviews.apache.org/r/58221/#comment246974>

    As part of shutdownAndAwaitTermination, shutdown is called first to stop new tasks to getting scheduled. 
    
    Later awaitTermination is called with timeout value while actually the interval at which the tasks are scheduled giving it enough time to terminate before calling shutdownNow.
    
    When awaitTermination is called for the second time to log an error, i feel the timeout can be 0, as we need not give any more time for the task to temrinate.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Line 172 (original), 172 (patched)
<https://reviews.apache.org/r/58221/#comment246975>

    Understand that this delay is causing issues during tests but making it 0 may not be a best choice.
    
    It may take some time for the server to initialize the database and be completely up to handle the notifications. 
    
    If 60000 milli sec is higher value, we should have some smaller value instead.


- kalyan kumar kalvagadda


On May 4, 2017, 5:48 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 4, 2017, 5:48 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/24/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated May 4, 2017, 5:48 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 


Diff: https://reviews.apache.org/r/58221/diff/24/

Changes: https://reviews.apache.org/r/58221/diff/23-24/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On May 4, 2017, 3:07 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707216#file1707216line75>
> >
> >     This shouldn't be here. It is the code from another JIRA.
> 
> Na Li wrote:
>     I can remove it

I need it here to avoid unit test failure when using local meta store. Since my code change changes the timing, it is more likely to use local meta store if we don't prevent it here.


> On May 4, 2017, 3:07 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 95 (original)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707216#file1707216line96>
> >
> >     Please add a comment explaining why these two constructors are different.

Kalyan mentioned that he has created a Jira to address the difference of the constructors, so can I leave it as it is for this jira?


> On May 4, 2017, 3:07 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Line 172 (original), 172 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707219#file1707219line172>
> >
> >     Why do we need this delay in the first place? Can it be 0?

I think the reason it was very large is because if the Hive meta store is not up yet, HMSFollower could connect to the local meta store, and the test will fail. So large initial delay is a work around of this issue. Since we avoid connect to the local meta store, we don't need such large delay any more.


- Na


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On May 4, 2017, 3:07 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707216#file1707216line75>
> >
> >     This shouldn't be here. It is the code from another JIRA.

I can remove it


> On May 4, 2017, 3:07 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 132 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707216#file1707216line138>
> >
> >     This shouldn't be here - it is a code from another JIRA

If I remove this code, HMSFollwer will use Hive local meta store, and the unit test will fail


- Na


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 99549bc 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 2ebe561 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 212c465 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java b5247d0 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




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

    This shouldn't be here. It is the code from another JIRA.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 95 (original)
<https://reviews.apache.org/r/58221/#comment246911>

    Please add a comment explaining why these two constructors are different.



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

    This shouldn't be here - it is a code from another JIRA



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 313 (patched)
<https://reviews.apache.org/r/58221/#comment246927>

    We may end up with hmsfollower without the executor here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 314 (patched)
<https://reviews.apache.org/r/58221/#comment246925>

    finally should be on previous line



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 318 (patched)
<https://reviews.apache.org/r/58221/#comment246924>

    It can't be null here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 322 (patched)
<https://reviews.apache.org/r/58221/#comment246928>

    This should be moved to the finally clause of the previous try block



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 326 (patched)
<https://reviews.apache.org/r/58221/#comment246926>

    catch should be on previous line



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 331 (patched)
<https://reviews.apache.org/r/58221/#comment246932>

    Since we are using conf, for consistency let's add conf to the arguments



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 332 (patched)
<https://reviews.apache.org/r/58221/#comment246929>

    This can be asserted with precondition



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 363 (patched)
<https://reviews.apache.org/r/58221/#comment246930>

    we should set sentrystorecleanservice to null here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 368 (patched)
<https://reviews.apache.org/r/58221/#comment246931>

    we an assert this with precondition



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 24 (patched)
<https://reviews.apache.org/r/58221/#comment246916>

    Please avoid wildcard imports for such utility classes, import what you need.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 157 (patched)
<https://reviews.apache.org/r/58221/#comment246919>

    Nit, this is a full sentence, so should start with a capital letter and end with a dot.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 165 (patched)
<https://reviews.apache.org/r/58221/#comment246917>

    This can be package-private
    
    Also, please use shorter line. Official limit is 120.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 166 (patched)
<https://reviews.apache.org/r/58221/#comment246918>

    Why would we call it with pool = null? This probably should be Preconditions.assertNonNull.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 177 (patched)
<https://reviews.apache.org/r/58221/#comment246921>

    This can be replaced with be {} instead of %s and format.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 180 (patched)
<https://reviews.apache.org/r/58221/#comment246922>

    rename ie to ignored



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Line 172 (original), 172 (patched)
<https://reviews.apache.org/r/58221/#comment246913>

    Why do we need this delay in the first place? Can it be 0?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 421 (original), 424 (patched)
<https://reviews.apache.org/r/58221/#comment246914>

    Can you explain this change?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 441 (original), 444 (patched)
<https://reviews.apache.org/r/58221/#comment246915>

    Please use string constant for col1_role instead of copying it in multiple places.


- Alexander Kolbasov


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On May 4, 2017, 5:47 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 322 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707217#file1707217line352>
> >
> >     it would be easier to read the code by keeping this section outside of previous finally block

This isn't about being easier to read. What may happen is that you get exception and the rest of the code isn't executed. The question is what code *must* be executed.


> On May 4, 2017, 5:47 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
> > Lines 165 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707218#file1707218line165>
> >
> >     Other functions in this class are public. Why do we want to limit it to just package private?

Usually we do not want to make anything public unless we need to. This particular function is new and there is no need for it to be public.


- Alexander


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


On May 4, 2017, 5:48 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 4, 2017, 5:48 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/24/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 95 (original)
<https://reviews.apache.org/r/58221/#comment246936>

    I need to revert it back. The code should be in constructor here, not in the constructor used for testing.



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

    I will remove it



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 171 (original), 171 (patched)
<https://reviews.apache.org/r/58221/#comment246938>

    I will move it to start service



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 313 (patched)
<https://reviews.apache.org/r/58221/#comment246940>

    I will check both



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 314 (patched)
<https://reviews.apache.org/r/58221/#comment246941>

    will update



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 318 (patched)
<https://reviews.apache.org/r/58221/#comment246942>

    it does not hurt to check here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 322 (patched)
<https://reviews.apache.org/r/58221/#comment246943>

    it would be easier to read the code by keeping this section outside of previous finally block



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 326 (patched)
<https://reviews.apache.org/r/58221/#comment246944>

    will move



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 331 (patched)
<https://reviews.apache.org/r/58221/#comment246945>

    will add



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 332 (patched)
<https://reviews.apache.org/r/58221/#comment246946>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 363 (patched)
<https://reviews.apache.org/r/58221/#comment246947>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 368 (patched)
<https://reviews.apache.org/r/58221/#comment246948>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 24 (patched)
<https://reviews.apache.org/r/58221/#comment246950>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 157 (patched)
<https://reviews.apache.org/r/58221/#comment246951>

    Will fix



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 165 (patched)
<https://reviews.apache.org/r/58221/#comment246952>

    Other functions in this class are public. Why do we want to limit it to just package private?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 166 (patched)
<https://reviews.apache.org/r/58221/#comment246953>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 177 (patched)
<https://reviews.apache.org/r/58221/#comment246954>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 180 (patched)
<https://reviews.apache.org/r/58221/#comment246955>

    will change



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Line 172 (original), 172 (patched)
<https://reviews.apache.org/r/58221/#comment246957>

    I believe it can be zero if it does not create local meta store.



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 421 (original), 424 (patched)
<https://reviews.apache.org/r/58221/#comment246958>

    I want to avoid two tests interfere with each other if they are running in parallel. I was trying to check what caused the test failure



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 441 (original), 444 (patched)
<https://reviews.apache.org/r/58221/#comment246959>

    will do


- Na Li


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 171 (original), 171 (patched)
<https://reviews.apache.org/r/58221/#comment246933>

    It looks like we can move it back to the start service since you fixed the problem with HMSFollower.


- Alexander Kolbasov


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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




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

    We have to move the code here to avoid running full snapshot multiple times. after the first time, getting empty full snapshot at run() will cause exception. 
    
    SENTRY-1712 Add capability to force Sentry to get full snapshot from HMS will provide more flexibility to get full snapshot.
    
    Example of the exception if this code is put in run() instead of constructor.
    
    2017-05-03 09:58:42,209 (pool-5-thread-1) [ERROR - org.apache.sentry.service.thrift.HMSFollower.run(HMSFollower.java:304)] Caught unexpected exception in HMSFollower! Caused by: AuthzObj: default already exists
    org.apache.sentry.core.common.exception.SentryAlreadyExistsException: AuthzObj: default already exists
    	at org.apache.sentry.provider.db.service.persistent.SentryStore.createAuthzPathsMappingCore(SentryStore.java:2541)
    	at org.apache.sentry.provider.db.service.persistent.SentryStore.access$3000(SentryStore.java:101)
    	at org.apache.sentry.provider.db.service.persistent.SentryStore$42.execute(SentryStore.java:2515)
    	at org.apache.sentry.provider.db.service.persistent.TransactionManager.executeTransaction(TransactionManager.java:114)
    	at org.apache.sentry.provider.db.service.persistent.TransactionManager.executeTransactionWithRetry(TransactionManager.java:181)
    	at org.apache.sentry.provider.db.service.persistent.SentryStore.persistFullPathsImage(SentryStore.java:2511)
    	at org.apache.sentry.service.thrift.HMSFollower.run(HMSFollower.java:269)
    	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
    	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
    	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
    	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)



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Line 172 (original), 172 (patched)
<https://reviews.apache.org/r/58221/#comment246829>

    since we start HMSFollower in SentryService.runServer() now, having a long delay will cause the unit test to fail. The reason is that HMSFollower has not run before the test starts and finishes, so no permission is obtained.


- Na Li


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated May 3, 2017, 7:20 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 


Diff: https://reviews.apache.org/r/58221/diff/23/

Changes: https://reviews.apache.org/r/58221/diff/22-23/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 25, 2017, 9:15 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 


Diff: https://reviews.apache.org/r/58221/diff/22/

Changes: https://reviews.apache.org/r/58221/diff/21-22/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 25, 2017, 3:17 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 


Diff: https://reviews.apache.org/r/58221/diff/21/

Changes: https://reviews.apache.org/r/58221/diff/20-21/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 25, 2017, 6:53 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 280 (patched)
> > <https://reviews.apache.org/r/58221/diff/20/?file=1698641#file1698641line315>
> >
> >     What if (hmsFollowerExecutor == null && hmsFollower != null) ?
> >     
> >     In this case you are going to override your existing HMSFollower with a new one. This should never happen though because you set it to null in close.
> >     
> >     So I think you can simplify the check and if you want, you can add a precondition that hmsFollower is null.

changed accordingly


- Na


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


On April 25, 2017, 3:17 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 3:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/21/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 25, 2017, 6:53 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 306 (patched)
> > <https://reviews.apache.org/r/58221/diff/20/?file=1698641#file1698641line341>
> >
> >     We are doing this twice - it is worth putting it in a helper function.

It is in SentryServiceUtility


- Na


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


On April 25, 2017, 9:15 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 9:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/22/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 280 (patched)
<https://reviews.apache.org/r/58221/#comment245918>

    What if (hmsFollowerExecutor == null && hmsFollower != null) ?
    
    In this case you are going to override your existing HMSFollower with a new one. This should never happen though because you set it to null in close.
    
    So I think you can simplify the check and if you want, you can add a precondition that hmsFollower is null.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 306 (patched)
<https://reviews.apache.org/r/58221/#comment245919>

    We are doing this twice - it is worth putting it in a helper function.


- Alexander Kolbasov


On April 24, 2017, 8:43 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:43 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/20/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 24, 2017, 8:43 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/20/

Changes: https://reviews.apache.org/r/58221/diff/19-20/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 20, 2017, 7:26 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/19/

Changes: https://reviews.apache.org/r/58221/diff/18-19/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 20, 2017, 12:33 a.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 312 (original), 320 (patched)
> > <https://reviews.apache.org/r/58221/diff/17-18/?file=1694265#file1694265line324>
> >
> >     This is getting trickier. Don't we have to call awaitTermination() after this?

We do now.


- Na


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


On April 25, 2017, 9:15 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 9:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/22/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

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



LGTM otherwise.


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 285 (original), 291 (patched)
<https://reviews.apache.org/r/58221/#comment245557>

    nit. 
    Could not start the "periodic executor of" HMSFollower.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 312 (original), 320 (patched)
<https://reviews.apache.org/r/58221/#comment245558>

    This is getting trickier. Don't we have to call awaitTermination() after this?


- Vamsee Yarlagadda


On April 20, 2017, 12:17 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 20, 2017, 12:17 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/18/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 20, 2017, 12:17 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/18/

Changes: https://reviews.apache.org/r/58221/diff/17-18/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 19, 2017, 9 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 321 (patched)
> > <https://reviews.apache.org/r/58221/diff/17/?file=1694265#file1694265line353>
> >
> >     I think you may need to wait for termination of HMSFOllower here. Otherwice it may be still running when you call close() and this may cause troubles.
> 
> Na Li wrote:
>     Do you mean waiting for termination of hmsFollowerExecutor before call HMSFOllower.close()?
>     
>     The code has tried to wait for termination of the executor, and only close HMSFollower at end. What else can I do to wait further?
>     
>         long timeoutValue = conf.getLong(ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS,
>                 ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS_DEFAULT);
>         try {
>           hmsFollowerExecutor.shutdown();
>           boolean terminated = hmsFollowerExecutor.awaitTermination(timeoutValue, TimeUnit.MILLISECONDS);
>           if (!terminated) {
>             hmsFollowerExecutor.shutdownNow();
>           }
>         } catch (SecurityException e) {
>           LOGGER.error("MSFollower executor shutdown failed at SecurityException which should never happen", e);
>         } catch (InterruptedException e) {
>           LOGGER.error("Could not await HMSFollower executor to terminate", e);
>     
>           try {
>             hmsFollowerExecutor.shutdownNow();
>           } catch (SecurityException ex) {
>             LOGGER.error("HMSFollower executor shutdownNow failed at SecurityException which should never happen", ex);
>           }
>         }
>         finally {
>           hmsFollowerExecutor = null;
>     
>           // close connections
>           if (hmsFollower != null) {
>             hmsFollower.close();
>           }
>         }

I added "hmsFollowerExecutor.awaitTermination(timeoutValue, TimeUnit.MILLISECONDS)" after shutdownNow().


- Na


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


On April 20, 2017, 12:17 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 20, 2017, 12:17 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/18/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 19, 2017, 9 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 321 (patched)
> > <https://reviews.apache.org/r/58221/diff/17/?file=1694265#file1694265line353>
> >
> >     I think you may need to wait for termination of HMSFOllower here. Otherwice it may be still running when you call close() and this may cause troubles.

Do you mean waiting for termination of hmsFollowerExecutor before call HMSFOllower.close()?

The code has tried to wait for termination of the executor, and only close HMSFollower at end. What else can I do to wait further?

    long timeoutValue = conf.getLong(ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS,
            ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS_DEFAULT);
    try {
      hmsFollowerExecutor.shutdown();
      boolean terminated = hmsFollowerExecutor.awaitTermination(timeoutValue, TimeUnit.MILLISECONDS);
      if (!terminated) {
        hmsFollowerExecutor.shutdownNow();
      }
    } catch (SecurityException e) {
      LOGGER.error("MSFollower executor shutdown failed at SecurityException which should never happen", e);
    } catch (InterruptedException e) {
      LOGGER.error("Could not await HMSFollower executor to terminate", e);

      try {
        hmsFollowerExecutor.shutdownNow();
      } catch (SecurityException ex) {
        LOGGER.error("HMSFollower executor shutdownNow failed at SecurityException which should never happen", ex);
      }
    }
    finally {
      hmsFollowerExecutor = null;

      // close connections
      if (hmsFollower != null) {
        hmsFollower.close();
      }
    }


- Na


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 19, 2017, 9 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 321 (patched)
> > <https://reviews.apache.org/r/58221/diff/17/?file=1694265#file1694265line353>
> >
> >     I think you may need to wait for termination of HMSFOllower here. Otherwice it may be still running when you call close() and this may cause troubles.
> 
> Na Li wrote:
>     Do you mean waiting for termination of hmsFollowerExecutor before call HMSFOllower.close()?
>     
>     The code has tried to wait for termination of the executor, and only close HMSFollower at end. What else can I do to wait further?
>     
>         long timeoutValue = conf.getLong(ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS,
>                 ServerConfig.SENTRY_HMSFOLLOWER_INTERVAL_MILLS_DEFAULT);
>         try {
>           hmsFollowerExecutor.shutdown();
>           boolean terminated = hmsFollowerExecutor.awaitTermination(timeoutValue, TimeUnit.MILLISECONDS);
>           if (!terminated) {
>             hmsFollowerExecutor.shutdownNow();
>           }
>         } catch (SecurityException e) {
>           LOGGER.error("MSFollower executor shutdown failed at SecurityException which should never happen", e);
>         } catch (InterruptedException e) {
>           LOGGER.error("Could not await HMSFollower executor to terminate", e);
>     
>           try {
>             hmsFollowerExecutor.shutdownNow();
>           } catch (SecurityException ex) {
>             LOGGER.error("HMSFollower executor shutdownNow failed at SecurityException which should never happen", ex);
>           }
>         }
>         finally {
>           hmsFollowerExecutor = null;
>     
>           // close connections
>           if (hmsFollower != null) {
>             hmsFollower.close();
>           }
>         }
> 
> Na Li wrote:
>     I added "hmsFollowerExecutor.awaitTermination(timeoutValue, TimeUnit.MILLISECONDS)" after shutdownNow().

Sasha, can you take a look at this and see if it is fixed?


- Na


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


On April 25, 2017, 9:15 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 9:15 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 57b7f88 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/22/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 321 (patched)
<https://reviews.apache.org/r/58221/#comment245503>

    I think you may need to wait for termination of HMSFOllower here. Otherwice it may be still running when you call close() and this may cause troubles.


- Alexander Kolbasov


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 19, 2017, 2:23 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/17/

Changes: https://reviews.apache.org/r/58221/diff/16-17/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 19, 2017, 3:25 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/16/

Changes: https://reviews.apache.org/r/58221/diff/15-16/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Vamsee Yarlagadda <va...@cloudera.com>.


> On April 18, 2017, 9:15 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 275 (patched)
> > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line307>
> >
> >     I know it doesn't hurt to check for hmsFollower != null but to make the code more readable can we replace this constraint with (notificationLogEnabled == true)?
> 
> Na Li wrote:
>     check notification itself is not enough. If hmsFollower is null, we don't want to continue scheduling its task. 
>     
>     I was checking both notifcation and hmsFollower. As you mentioned, checking notificationLogEnabled makes code more readable. But Kalyan wants to get rid of it to make the code simplier. 
>     
>     So how can I satisfy both preference? Or you two can reach agreement?
> 
> Na Li wrote:
>     Sasha mentioned that notificationLogEnabled will be gone soon as it will always be true. If it is false, Hive and Sentry won't work together.

The reason why I left my comment was - Looking at the code, I realized that hmsFollower can never be null if the constructor of SentryService passed. So i thought it is not really necessarily to check whether it is null or not.

But looking at your comment pointing to Sasha's note that this notificationLogEnabled flag will soon go away, then we can leave it as is to avoid making changes again.


- Vamsee


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 18, 2017, 9:15 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 275 (patched)
> > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line307>
> >
> >     I know it doesn't hurt to check for hmsFollower != null but to make the code more readable can we replace this constraint with (notificationLogEnabled == true)?

check notification itself is not enough. If hmsFollower is null, we don't want to continue scheduling its task. 

I was checking both notifcation and hmsFollower. As you mentioned, checking notificationLogEnabled makes code more readable. But Kalyan wants to get rid of it to make the code simplier. 

So how can I satisfy both preference? Or you two can reach agreement?


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 18, 2017, 9:15 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 275 (patched)
> > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line307>
> >
> >     I know it doesn't hurt to check for hmsFollower != null but to make the code more readable can we replace this constraint with (notificationLogEnabled == true)?
> 
> Na Li wrote:
>     check notification itself is not enough. If hmsFollower is null, we don't want to continue scheduling its task. 
>     
>     I was checking both notifcation and hmsFollower. As you mentioned, checking notificationLogEnabled makes code more readable. But Kalyan wants to get rid of it to make the code simplier. 
>     
>     So how can I satisfy both preference? Or you two can reach agreement?

Sasha mentioned that notificationLogEnabled will be gone soon as it will always be true. If it is false, Hive and Sentry won't work together.


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

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


Fix it, then Ship it!




LGTM Otherwise


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 275 (patched)
<https://reviews.apache.org/r/58221/#comment245364>

    I know it doesn't hurt to check for hmsFollower != null but to make the code more readable can we replace this constraint with (notificationLogEnabled == true)?


- Vamsee Yarlagadda


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 18, 2017, 3:46 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/15/

Changes: https://reviews.apache.org/r/58221/diff/14-15/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 17, 2017, 8:11 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/14/

Changes: https://reviews.apache.org/r/58221/diff/13-14/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 17, 2017, 6:48 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/13/

Changes: https://reviews.apache.org/r/58221/diff/12-13/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 17, 2017, 5:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 107 (patched)
> > <https://reviews.apache.org/r/58221/diff/11/?file=1692818#file1692818line107>
> >
> >     Please add comment explaining how do you use the future.

I will remove the future since in the latest build, calling startSentryStoreCleaner only at runServer() does not cause test failure. So I will simplify the code.


> On April 17, 2017, 5:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 282 (patched)
> > <https://reviews.apache.org/r/58221/diff/11/?file=1692818#file1692818line308>
> >
> >     Please add clarifying parenthesis:
> >     
> >     if (notificationLogEnabled && (hmsFollowerExecutor == null) && (hmsFollower != null)) {

It is done


> On April 17, 2017, 5:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 292 (patched)
> > <https://reviews.apache.org/r/58221/diff/11/?file=1692818#file1692818line318>
> >
> >     We can't handle this - if HMSFollowert should start and it doesn't, SentryService should fail. The easiest thing may be just to propagate the exception up the chain.

I rethrow the exception


> On April 17, 2017, 5:55 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 343 (patched)
> > <https://reviews.apache.org/r/58221/diff/11/?file=1692818#file1692818line369>
> >
> >     You can move this outside the try block and return immediately if storeCleanPeriodSecs <= 0. This will simplify the code a bit.

it is done.


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 107 (patched)
<https://reviews.apache.org/r/58221/#comment245179>

    Please add comment explaining how do you use the future.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 282 (patched)
<https://reviews.apache.org/r/58221/#comment245181>

    Please add clarifying parenthesis:
    
    if (notificationLogEnabled && (hmsFollowerExecutor == null) && (hmsFollower != null)) {



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 292 (patched)
<https://reviews.apache.org/r/58221/#comment245183>

    We can't handle this - if HMSFollowert should start and it doesn't, SentryService should fail. The easiest thing may be just to propagate the exception up the chain.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 343 (patched)
<https://reviews.apache.org/r/58221/#comment245184>

    You can move this outside the try block and return immediately if storeCleanPeriodSecs <= 0. This will simplify the code a bit.


- Alexander Kolbasov


On April 17, 2017, 5:55 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 17, 2017, 5:55 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/12/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 17, 2017, 5:55 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 


Diff: https://reviews.apache.org/r/58221/diff/12/

Changes: https://reviews.apache.org/r/58221/diff/11-12/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 14, 2017, 8:36 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/11/

Changes: https://reviews.apache.org/r/58221/diff/10-11/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> >     I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.
> 
> Na Li wrote:
>     The original code to start HMSFollower is in constructor. When I move it to runServer(), putting it at the beginning is the closest approach to the original in terms of the ordering. I am not familar with the system, and moving it to the end of runServer() may cause some unknown problems.
> 
> Na Li wrote:
>     I will move it to the end of runServer()

there is regression and test org.apache.sentry.tests.e2e.metastore.TestDbNotificationListenerSentryDeserializer.testAlterPartition failed after moving the function to the end of runServer(). So I move it back to the beginning of runServer().


- Na


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> >     I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.
> 
> Na Li wrote:
>     The original code to start HMSFollower is in constructor. When I move it to runServer(), putting it at the beginning is the closest approach to the original in terms of the ordering. I am not familar with the system, and moving it to the end of runServer() may cause some unknown problems.
> 
> Na Li wrote:
>     I will move it to the end of runServer()
> 
> Na Li wrote:
>     there is regression and test org.apache.sentry.tests.e2e.metastore.TestDbNotificationListenerSentryDeserializer.testAlterPartition failed after moving the function to the end of runServer(). So I move it back to the beginning of runServer().

Why do you think that it is better to start HMSFollower in the end?


- Alexander


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 375 (original), 474 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line503>
> >
> >     SentryStore cleaner need not be restarted every time sentry service is stoped and started.
> 
> Na Li wrote:
>     Since we have to stop SentryStore cleaner at stop(). When test calls start() after stop(), we have to start it in start(). Otherwise, it won't work, and will cause test failure.

arguments from Sasha for why shutdown the executors at stop() "[1:19 AM] Alexander Kolbasov: In normal (non-test) situation stop() method is called during regular shutdown - e.g. when ^C is pressed (SIGINT is received by the process). As this happens we need to shutdown executors. This is the regular use case. Test use case is probably different, but I wouldn't worry too much about performance for creating/destroying executors for tests. Since in the normal use case we do need to shutdown the executor and executor shutdown is a well-defined operation, I think it is cleaner to use it rather then rely on just stopping the service or doing something complicated with Future.cancel().


- Na


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> >     I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.
> 
> Na Li wrote:
>     The original code to start HMSFollower is in constructor. When I move it to runServer(), putting it at the beginning is the closest approach to the original in terms of the ordering. I am not familar with the system, and moving it to the end of runServer() may cause some unknown problems.
> 
> Na Li wrote:
>     I will move it to the end of runServer()
> 
> Na Li wrote:
>     there is regression and test org.apache.sentry.tests.e2e.metastore.TestDbNotificationListenerSentryDeserializer.testAlterPartition failed after moving the function to the end of runServer(). So I move it back to the beginning of runServer().
> 
> Alexander Kolbasov wrote:
>     Why do you think that it is better to start HMSFollower in the end?

I think it should be fine


- kalyan kumar


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


On May 4, 2017, 5:48 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated May 4, 2017, 5:48 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java ec8676e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java ce73358 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 834ed41 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/24/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 221 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line249>
> >
> >     SentryStore cleaner need not be restarted every time sentry service is stoped and started.
> 
> Na Li wrote:
>     in normal operation, stop() is the last call to clean up the resources. And we need to stop SentryStore cleaner.

Normally we don't know whether this is the stop() happening because we are exiting or this is a test. During exit we do need to shut down the executor service. There is no harm in restarting the service for tests.


- Alexander


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> >     I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.
> 
> Na Li wrote:
>     The original code to start HMSFollower is in constructor. When I move it to runServer(), putting it at the beginning is the closest approach to the original in terms of the ordering. I am not familar with the system, and moving it to the end of runServer() may cause some unknown problems.

I will move it to the end of runServer()


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 221 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line249>
> >
> >     SentryStore cleaner need not be restarted every time sentry service is stoped and started.

in normal operation, stop() is the last call to clean up the resources. And we need to stop SentryStore cleaner.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> >     I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.

The original code to start HMSFollower is in constructor. When I move it to runServer(), putting it at the beginning is the closest approach to the original in terms of the ordering. I am not familar with the system, and moving it to the end of runServer() may cause some unknown problems.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 373 (original), 472 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line501>
> >
> >     HMSFollower need not be stopped when sentry service is stoped.

in normal operation, stop() is the last call to clean up the resources. And we need to stop HMSFollower.


> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 375 (original), 474 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line503>
> >
> >     SentryStore cleaner need not be restarted every time sentry service is stoped and started.

Since we have to stop SentryStore cleaner at stop(). When test calls start() after stop(), we have to start it in start(). Otherwise, it won't work, and will cause test failure.


- Na


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 221 (patched)
<https://reviews.apache.org/r/58221/#comment245068>

    SentryStore cleaner need not be restarted every time sentry service is stoped and started.



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

    I feel we should start call startHMSFollower at the end of runServer function. If there are no issues in starting the sentry service, we can start HMSFolloeer at the end.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 373 (original), 472 (patched)
<https://reviews.apache.org/r/58221/#comment245070>

    HMSFollower need not be stopped when sentry service is stoped.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 375 (original), 474 (patched)
<https://reviews.apache.org/r/58221/#comment245069>

    SentryStore cleaner need not be restarted every time sentry service is stoped and started.


- kalyan kumar kalvagadda


On April 14, 2017, 3:41 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 3:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 14, 2017, 6:48 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 328 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line356>
> >
> >     HmsFollower is AutoClosable. It will call close. You need not explicitly call close.

The close() will be called if you use try-with-resource construct, but it wouldn't be called by itself.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 328 (patched)
<https://reviews.apache.org/r/58221/#comment245046>

    HmsFollower is AutoClosable. It will call close. You need not explicitly call close.


- kalyan kumar kalvagadda


On April 14, 2017, 3:41 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 3:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On April 14, 2017, 4:02 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 280 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line308>
> >
> >     If hmsFollower != null you can assume that notification log is enabled. I feel you don't have to make check it explicitly.

We'll remove notificationLogEnabled soon anyway since we don't support anything else.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 14, 2017, 4:02 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 280 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line308>
> >
> >     If hmsFollower != null you can assume that notification log is enabled. I feel you don't have to make check it explicitly.
> 
> Alexander Kolbasov wrote:
>     We'll remove notificationLogEnabled soon anyway since we don't support anything else.

The intention of the code is more obvious when checking notification log is enabled or not.


- Na


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


On April 18, 2017, 3:46 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 18, 2017, 3:46 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 280 (patched)
<https://reviews.apache.org/r/58221/#comment245045>

    If hmsFollower != null you can assume that notification log is enabled. I feel you don't have to make check it explicitly.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 326 (patched)
<https://reviews.apache.org/r/58221/#comment245043>

    curly braces be in the same line.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 333 (patched)
<https://reviews.apache.org/r/58221/#comment245044>

    curly braces be in the same line. This comments applies couple of places below. Make sure you correct it.


- kalyan kumar kalvagadda


On April 14, 2017, 3:41 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 3:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 280 (patched)
<https://reviews.apache.org/r/58221/#comment245072>

    I think it is safer to have both conditions there in case someone else decide to create hmsFollower regardless of the notification enabled or not.
    
    It also makes the intention of the code clear: the action should be done only when notification log is enabled.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 326 (patched)
<https://reviews.apache.org/r/58221/#comment245073>

    It's from c# coding style. I will udpate this.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 328 (patched)
<https://reviews.apache.org/r/58221/#comment245074>

    The close() method of an AutoCloseable object is called automatically when exiting a try-with-resources block for which the object has been declared in the resource specification header. This construction ensures prompt release, avoiding resource exhaustion exceptions and errors that may otherwise occur.
    
    HmsFollower is not created in a try-with-resources block, and we need to call its close() explicitely



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 333 (patched)
<https://reviews.apache.org/r/58221/#comment245075>

    will do


- Na Li


On April 14, 2017, 3:41 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 3:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 14, 2017, 3:41 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/10/

Changes: https://reviews.apache.org/r/58221/diff/9-10/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 13, 2017, 7:56 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/9/

Changes: https://reviews.apache.org/r/58221/diff/8-9/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 13, 2017, 3:37 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/8/

Changes: https://reviews.apache.org/r/58221/diff/7-8/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 13, 2017, 4:47 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/7/

Changes: https://reviews.apache.org/r/58221/diff/6-7/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 12, 2017, 5:38 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 16676fb 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/6/

Changes: https://reviews.apache.org/r/58221/diff/5-6/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 12, 2017, 3:47 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Changes
-------

1. Implement HMSFollower.close(). Remove its start() and stop().
2. Add SentryService.close(), executor service shutdown are moved there. in stop(), cancel the future of the tasks instead of shutting down executor services. 
3. In SentryService$CommandImpl.run(), call SentryService.stop() and close() instead of just call serviceExecutor.shutdown().


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/5/

Changes: https://reviews.apache.org/r/58221/diff/4-5/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 12, 2017, 3:46 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db63 


Diff: https://reviews.apache.org/r/58221/diff/4/

Changes: https://reviews.apache.org/r/58221/diff/3-4/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.


> On April 10, 2017, 6:50 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 312 (original), 317 (patched)
> > <https://reviews.apache.org/r/58221/diff/2-3/?file=1686253#file1686253line320>
> >
> >     Should we have a new config specific for timeout?

I thought about that option. Scheduling interval is a good upper limit on how long the task can run since the HMSFollower will be scheduled based on that interval. So I opt to not have another configuration for this operation.


> On April 10, 2017, 6:50 p.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 325 (patched)
> > <https://reviews.apache.org/r/58221/diff/3/?file=1686519#file1686519line337>
> >
> >     Should we call hmsFollower.stop(); before terminate hmsFollowerExecutor?

I thought about this option.

In the current approach, terminate HmsFollowerExecutor will stop scheduling HMSFollower. Then we can close all connections. There should be no issue.
If we close all connections first (call hmsFollower.stop(); before terminate hmsFollowerExecutor), it is possible that HMSFollower is scheduled, and then connections are created again. That is not desirable.


- Na


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


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Hao Hao <ha...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 312 (original), 317 (patched)
<https://reviews.apache.org/r/58221/#comment244399>

    Should we have a new config specific for timeout?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 325 (patched)
<https://reviews.apache.org/r/58221/#comment244400>

    Should we call hmsFollower.stop(); before terminate hmsFollowerExecutor?


- Hao Hao


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Alexander Kolbasov <ak...@gmail.com>.

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

Why do you want to actually recreate a new instance of HMSFollower every time you start a service? The only thing that's actually needed is to manage execution of the service, not the instance.

IMO HMSFollower should just be a singleton which is created once and can get scheduled or not depending on the need. You don't have to make it a singleton as part of this change (this would be nice though) but actually creating a new one every time seems to be not needed.

Since HMSFollower doesn't actually keep state - it is all stored in DB it doesn't make much sense to have multiple instances. And since it is runnable, adding start/stop methods to it doesn't seem right.

- Alexander Kolbasov

On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 7, 2017, 5:23 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 6c14f5e46aad4223347d8d057188d31efbb68ed8 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 


Diff: https://reviews.apache.org/r/58221/diff/3/

Changes: https://reviews.apache.org/r/58221/diff/2-3/


Testing
-------


Thanks,

Na Li

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

Posted by Na Li <li...@cloudera.com>.

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

(Updated April 6, 2017, 10:40 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

SENTRY-1649 move HMS follower to runServer


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 132db635f639cef91ce675d34d717e6125a0a4d1 


Diff: https://reviews.apache.org/r/58221/diff/2/

Changes: https://reviews.apache.org/r/58221/diff/1-2/


Testing
-------


Thanks,

Na Li