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 via Review Board <no...@reviews.apache.org> on 2018/06/26 20:00:49 UTC
Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java a4fa226
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
Diff: https://reviews.apache.org/r/67749/diff/1/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 12:50 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
> > Lines 224-233 (patched)
> > <https://reviews.apache.org/r/67749/diff/2/?file=2046227#file2046227line224>
> >
> > As sentry doesn't changes to configurations at runtime, we doesn't have to do this every time.
> > We can use constant that is initialized once and we can just use in setAuthorizable method.
> >
> > we need not pass the HiveAuthzConf while construncting SentryHmsEvent.
>
> Na Li wrote:
> I have changed to code to get server name at once and use it in following calls.
>
> For class encapsulation and future extention, it is still better to provide HiveAuthzConf to SentryHmsEvent than for caller to get the configuration and provide to it as input.
Lina, serverName need be not stored again in SentryHmsEvent. Why don't you pass serverName to setAuthorizable in the constructor directly?
Example:
public SentryHmsEvent(String inServerName, CreateDatabaseEvent event) {
this(event, EventType.CREATE_DATABASE);
setOwnerInfo(event.getDatabase());
setAuthorizable(event.getDatabase(), inServerName);
}
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205423
-----------------------------------------------------------
On June 27, 2018, 7:13 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 7:13 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/4/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 12:50 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
> > Lines 224-233 (patched)
> > <https://reviews.apache.org/r/67749/diff/2/?file=2046227#file2046227line224>
> >
> > As sentry doesn't changes to configurations at runtime, we doesn't have to do this every time.
> > We can use constant that is initialized once and we can just use in setAuthorizable method.
> >
> > we need not pass the HiveAuthzConf while construncting SentryHmsEvent.
I have changed to code to get server name at once and use it in following calls.
For class encapsulation and future extention, it is still better to provide HiveAuthzConf to SentryHmsEvent than for caller to get the configuration and provide to it as input.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205423
-----------------------------------------------------------
On June 27, 2018, 3:57 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 3:57 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/3/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 27, 2018, 12:50 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
> > Lines 224-233 (patched)
> > <https://reviews.apache.org/r/67749/diff/2/?file=2046227#file2046227line224>
> >
> > As sentry doesn't changes to configurations at runtime, we doesn't have to do this every time.
> > We can use constant that is initialized once and we can just use in setAuthorizable method.
> >
> > we need not pass the HiveAuthzConf while construncting SentryHmsEvent.
>
> Na Li wrote:
> I have changed to code to get server name at once and use it in following calls.
>
> For class encapsulation and future extention, it is still better to provide HiveAuthzConf to SentryHmsEvent than for caller to get the configuration and provide to it as input.
>
> kalyan kumar kalvagadda wrote:
> Lina, serverName need be not stored again in SentryHmsEvent. Why don't you pass serverName to setAuthorizable in the constructor directly?
> Example:
> public SentryHmsEvent(String inServerName, CreateDatabaseEvent event) {
> this(event, EventType.CREATE_DATABASE);
> setOwnerInfo(event.getDatabase());
> setAuthorizable(event.getDatabase(), inServerName);
> }
I saved it, so I don't need to provide it as input for each setAuthorizable(). What is the significent advantage of not saving a variable versus the current approach?
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205423
-----------------------------------------------------------
On June 27, 2018, 7:13 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 7:13 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/4/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205423
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 224-233 (patched)
<https://reviews.apache.org/r/67749/#comment288330>
As sentry doesn't changes to configurations at runtime, we doesn't have to do this every time.
We can use constant that is initialized once and we can just use in setAuthorizable method.
we need not pass the HiveAuthzConf while construncting SentryHmsEvent.
- kalyan kumar kalvagadda
On June 26, 2018, 10:35 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 10:35 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/2/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205451
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 100 (original), 100 (patched)
<https://reviews.apache.org/r/67749/#comment288370>
I really not like to pass Configuration objects between classes as it does not explicitly specify why we needed it. In this case, we only need it to get the server name.
We created the SentryHmsEvent to abstract these kind of cases, so we could keep it that way if we pass the server name and avoid passing the configuration.
You can get the server name from the constroctor of SentrySyncHMSNotificationsPostEventListenre where the configuratoin is passed as a parameter, and just pass the server string to the SentryHmsEvent.
- Sergio Pena
On June 27, 2018, 3:57 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 3:57 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/3/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205465
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On June 27, 2018, 7:13 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 7:13 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/4/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205486
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On June 27, 2018, 10:29 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 10:29 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 1b0de6a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java fe6389c
>
>
> Diff: https://reviews.apache.org/r/67749/diff/6/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
(Updated June 27, 2018, 10:29 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 1b0de6a
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java fe6389c
Diff: https://reviews.apache.org/r/67749/diff/6/
Changes: https://reviews.apache.org/r/67749/diff/5-6/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205480
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 83-88 (original), 87-92 (patched)
<https://reviews.apache.org/r/67749/#comment288393>
update the java doc.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 95-100 (original), 99-104 (patched)
<https://reviews.apache.org/r/67749/#comment288394>
update the java doc.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 110-115 (original), 116-121 (patched)
<https://reviews.apache.org/r/67749/#comment288395>
update the java doc.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 122-127 (original), 128-133 (patched)
<https://reviews.apache.org/r/67749/#comment288396>
update the java doc.
- kalyan kumar kalvagadda
On June 27, 2018, 10:05 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 10:05 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 1b0de6a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java fe6389c
>
>
> Diff: https://reviews.apache.org/r/67749/diff/5/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
(Updated June 27, 2018, 10:05 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 1b0de6a
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java fe6389c
Diff: https://reviews.apache.org/r/67749/diff/5/
Changes: https://reviews.apache.org/r/67749/diff/4-5/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
(Updated June 27, 2018, 7:13 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
Diff: https://reviews.apache.org/r/67749/diff/4/
Changes: https://reviews.apache.org/r/67749/diff/3-4/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
(Updated June 27, 2018, 3:57 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
Diff: https://reviews.apache.org/r/67749/diff/3/
Changes: https://reviews.apache.org/r/67749/diff/2-3/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/
-----------------------------------------------------------
(Updated June 26, 2018, 10:35 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2280
https://issues.apache.org/jira/browse/sentry-2280
Repository: sentry
Description
-------
The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java f7d1b07
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java 8e79cac
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
Diff: https://reviews.apache.org/r/67749/diff/2/
Changes: https://reviews.apache.org/r/67749/diff/1-2/
Testing
-------
Manually trace into code and verfied that now the server receives request and client receiveds response correctly
Thanks,
Na Li
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 26, 2018, 8:20 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
> > Lines 189 (patched)
> > <https://reviews.apache.org/r/67749/diff/1/?file=2045271#file2045271line189>
> >
> > The protocol is set automatically the TSentryHmsEventNotification() constructor. Why was this failing on the server side?
the flag is not set in default constructor, and thrift protocol only checks the flag, not real value. If I call this function, the flag is set.
public void setProtocol_version(int protocol_version) {
this.protocol_version = protocol_version;
setProtocol_versionIsSet(true);
}
public void setProtocol_versionIsSet(boolean value) {
__isset_bitfield = EncodingUtils.setBit(__isset_bitfield, __PROTOCOL_VERSION_ISSET_ID, value);
}
public boolean isSetProtocol_version() {
return EncodingUtils.testBit(__isset_bitfield, __PROTOCOL_VERSION_ISSET_ID);
}
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205392
-----------------------------------------------------------
On June 26, 2018, 8 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 8 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java a4fa226
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/1/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 26, 2018, 8:20 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
> > Lines 360-362 (original), 360-362 (patched)
> > <https://reviews.apache.org/r/67749/diff/1/?file=2045274#file2045274line360>
> >
> > Why was it moved in different order? I think all the fields, exceptio the protocl_version, should be optional to allow future compatibility if we decide to deprecate some of the fields.
the optional fields must be after required fields. We always need the other fields to function correctly, so they are required fields. We need to balance the flexibility and avoid too many checks for optional fields
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205392
-----------------------------------------------------------
On June 26, 2018, 8 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 8 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java a4fa226
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/1/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205392
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Line 158 (original), 161 (patched)
<https://reviews.apache.org/r/67749/#comment288279>
This should not be a constant. The server name is part of the authorization request from the client. For instance, take a look at listPrivilegesByRoleName() which has an Authorizable as a parameter that contains the server.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 189 (patched)
<https://reviews.apache.org/r/67749/#comment288281>
The protocol is set automatically the TSentryHmsEventNotification() constructor. Why was this failing on the server side?
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java
Lines 38 (patched)
<https://reviews.apache.org/r/67749/#comment288280>
server1 is not a constant. server1 is usually a value assigned to the server.
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 360-362 (original), 360-362 (patched)
<https://reviews.apache.org/r/67749/#comment288282>
Why was it moved in different order? I think all the fields, exceptio the protocl_version, should be optional to allow future compatibility if we decide to deprecate some of the fields.
- Sergio Pena
On June 26, 2018, 8 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 8 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java a4fa226
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/1/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 67749: SENTRY-2280: The request received in
SentryPolicyStoreProcessor.sentry_notify_hms_event is null.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67749/#review205391
-----------------------------------------------------------
Fix it.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Line 158 (original), 161 (patched)
<https://reviews.apache.org/r/67749/#comment288276>
I think this should not be hard coded. Instead it should be derived from configuration property "hive.sentry.server".
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Line 166 (original), 169 (patched)
<https://reviews.apache.org/r/67749/#comment288277>
I think this should not be hard coded. Instead it should be derived from configuration property "hive.sentry.server".
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java
Lines 38 (patched)
<https://reviews.apache.org/r/67749/#comment288278>
Not needed.
- kalyan kumar kalvagadda
On June 26, 2018, 8 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67749/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 8 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2280
> https://issues.apache.org/jira/browse/sentry-2280
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The required fields in TSentryHmsEventNotification and TSentryHmsEventNotificationResponse are not filled, which caused the request from client to sever to be null. And the response from server to client is null. Change the data structure to make owner info optional, and fill other required fields properly
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java 42be3c3
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessConstants.java a4fa226
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java 75b2799
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift 56aedcb
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 3927150
>
>
> Diff: https://reviews.apache.org/r/67749/diff/1/
>
>
> Testing
> -------
>
> Manually trace into code and verfied that now the server receives request and client receiveds response correctly
>
>
> Thanks,
>
> Na Li
>
>