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/05/12 19:38:51 UTC
Review Request 59241: SENTRY-1705: Do not start HMSFollower if Hive
isn't configured
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: SENTRY-1705
https://issues.apache.org/jira/browse/SENTRY-1705
Repository: sentry
Description
-------
check if metastore uri is configured or not. If not, don't create HMSFollower.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
Diff: https://reviews.apache.org/r/59241/diff/1/
Testing
-------
Thanks,
Na Li
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review174958
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 283-288 (patched)
<https://reviews.apache.org/r/59241/#comment248262>
This block is also used on stopHMSFollower. Should we create a method to check this? Some like SentryServiceUtil.isHiveMetastoreURISet()? or something like that.
- Sergio Pena
On May 15, 2017, 3:52 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 15, 2017, 3:52 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
>
>
> Diff: https://reviews.apache.org/r/59241/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
> On May 19, 2017, 12:02 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 324 (patched)
> > <https://reviews.apache.org/r/59241/diff/3/?file=1724164#file1724164line324>
> >
> > So we no longer have the situation where metastore URI isn't present initially but later becomes available?
Yes for those tests associated to TestHDFSIntegrationBase or TestHDFSIntegration. The sentry server is started after hive-site.xml is there.
If in other tests, the sentry server is started without hive-site.xml, it is possible metastore URI isn't present initially but later becomes available. I will check if there are any test like this.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review175452
-----------------------------------------------------------
On May 18, 2017, 9:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 9:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
>
>
> Diff: https://reviews.apache.org/r/59241/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
> On May 19, 2017, 12:02 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 137 (original), 136 (patched)
> > <https://reviews.apache.org/r/59241/diff/3/?file=1724163#file1724163line137>
> >
> > We already have this check in SentryService - we shouldn't even be here if it isn't present so why do we need to check it here?
I will remove it
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review175452
-----------------------------------------------------------
On May 18, 2017, 9:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 9:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
>
>
> Diff: https://reviews.apache.org/r/59241/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review175452
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 137 (original), 136 (patched)
<https://reviews.apache.org/r/59241/#comment248901>
We already have this check in SentryService - we shouldn't even be here if it isn't present so why do we need to check it here?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 324 (patched)
<https://reviews.apache.org/r/59241/#comment248903>
So we no longer have the situation where metastore URI isn't present initially but later becomes available?
- Alexander Kolbasov
On May 18, 2017, 9:04 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 9:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
>
>
> Diff: https://reviews.apache.org/r/59241/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review175472
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On May 19, 2017, 4:35 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 19, 2017, 4:35 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java e2fb36a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 0ca7704
>
>
> Diff: https://reviews.apache.org/r/59241/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/
-----------------------------------------------------------
(Updated May 19, 2017, 4:35 a.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: SENTRY-1705
https://issues.apache.org/jira/browse/SENTRY-1705
Repository: sentry
Description
-------
check if metastore uri is configured or not. If not, don't create HMSFollower.
This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java e2fb36a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 0ca7704
Diff: https://reviews.apache.org/r/59241/diff/4/
Changes: https://reviews.apache.org/r/59241/diff/3-4/
Testing
-------
Thanks,
Na Li
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/
-----------------------------------------------------------
(Updated May 18, 2017, 9:04 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: SENTRY-1705
https://issues.apache.org/jira/browse/SENTRY-1705
Repository: sentry
Description
-------
check if metastore uri is configured or not. If not, don't create HMSFollower.
This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 5dc0ad3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 35289aa
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9470437
Diff: https://reviews.apache.org/r/59241/diff/3/
Changes: https://reviews.apache.org/r/59241/diff/2-3/
Testing
-------
Thanks,
Na Li
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/
-----------------------------------------------------------
(Updated May 15, 2017, 3:52 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: SENTRY-1705
https://issues.apache.org/jira/browse/SENTRY-1705
Repository: sentry
Description
-------
check if metastore uri is configured or not. If not, don't create HMSFollower.
This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
Diff: https://reviews.apache.org/r/59241/diff/2/
Changes: https://reviews.apache.org/r/59241/diff/1-2/
Testing
-------
Thanks,
Na Li
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
> On May 13, 2017, 1:15 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 285 (patched)
> > <https://reviews.apache.org/r/59241/diff/1/?file=1717148#file1717148line285>
> >
> > It is better to capitalize as metastoreURI.
> > Should we also check for isEmoty()?
> >
> > We discovered before that initially (during tests) it is empty but later may become non-empty - why this isn't an issue any more?
This change depends on checking in sentry-1757. After that, the order to create services in test has changed to be similar to the production. So when sentry service starts, the configuration is already available. Then if the metaStoreURI is null, it means hive configuration at sentry is not present
> On May 13, 2017, 1:15 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 319 (patched)
> > <https://reviews.apache.org/r/59241/diff/1/?file=1717148#file1717148line319>
> >
> > Please don't depend on this here, but depend on known object state (e.g. hmsFollower being null).
I will check if they are both null. If so, skip the following processing
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review174877
-----------------------------------------------------------
On May 12, 2017, 7:49 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 7:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
>
>
> Diff: https://reviews.apache.org/r/59241/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/#review174877
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 285 (patched)
<https://reviews.apache.org/r/59241/#comment248104>
It is better to capitalize as metastoreURI.
Should we also check for isEmoty()?
We discovered before that initially (during tests) it is empty but later may become non-empty - why this isn't an issue any more?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 319 (patched)
<https://reviews.apache.org/r/59241/#comment248105>
Please don't depend on this here, but depend on known object state (e.g. hmsFollower being null).
- Alexander Kolbasov
On May 12, 2017, 7:49 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59241/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 7:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: SENTRY-1705
> https://issues.apache.org/jira/browse/SENTRY-1705
>
>
> Repository: sentry
>
>
> Description
> -------
>
> check if metastore uri is configured or not. If not, don't create HMSFollower.
>
> This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
>
>
> Diff: https://reviews.apache.org/r/59241/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59241: SENTRY-1705: Do not start HMSFollower if
Hive isn't configured
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59241/
-----------------------------------------------------------
(Updated May 12, 2017, 7:49 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: SENTRY-1705
https://issues.apache.org/jira/browse/SENTRY-1705
Repository: sentry
Description (updated)
-------
check if metastore uri is configured or not. If not, don't create HMSFollower.
This code change is base on assumption that code change in https://reviews.apache.org/r/59120/diff/5#1 is checked in (hive configuration is available when sentry server starts). Otherwise, it may break tests since in test, hive configuration is not available when sentry server starts.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6fb224c
Diff: https://reviews.apache.org/r/59241/diff/1/
Testing
-------
Thanks,
Na Li