You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/07/27 05:09:11 UTC
Review Request 36832: Refactor the AuditMetadataLogEntity to support
the audit log for generic mdoel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/
-----------------------------------------------------------
Review request for sentry, Dapeng Sun and shen guoquan.
Repository: sentry
Description
-------
Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
Diff: https://reviews.apache.org/r/36832/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Dapeng Sun <da...@intel.com>.
> On 七月 27, 2015, 2:43 p.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 29
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line29>
> >
> > I think Impala may also use **HiveAuditMetadataLogEntity**, Do you think **DbAuditMetadataLogEntity** is more suitable?
>
> Colin Ma wrote:
> thanks for the comments, HiveAuditMetadataLogEntity is only for hive, and there is a GMAuditMetadataLogEntity for generic model. All these should be DbAuditMetadataLogEntity, so I think HiveAuditMetadataLogEntity is better.
I found audit log is working at **SentryPolicyStoreProcessor**, it's not specified for HIVE only.
> On 七月 27, 2015, 2:43 p.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 109
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line109>
> >
> > If we return the new StringWriter, will the audit log still work? If not, we may need to throw the exception.
>
> Colin Ma wrote:
> The code here is to process the problems when write the audit log, and just output an empty audit log. I think the exception shouldn't be thrown here and log the problem is ok, because the audit log's problem shouldn't block the normal authorization process.
I don't sure what is the best practice, +0 about issue.
> On 七月 27, 2015, 2:43 p.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 37
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line37>
> >
> > Is there any place need constructor without field? if not, we may need to delete it or mark it private
>
> Colin Ma wrote:
> Yes, this constructor is used in core function and test case.
I found the usage at **JsonLogEntityFactory** and **TestJsonLogEntityFactory**. Since AuditMetadataLogEntity has been changed to **abstract**. I think we may also need to update the two classes in this jira.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review93086
-----------------------------------------------------------
On 七月 28, 2015, 9:14 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated 七月 28, 2015, 9:14 a.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: Refactor the AuditMetadataLogEntity to
support the audit log for generic mdoel
Posted by Colin Ma <ju...@intel.com>.
> On July 27, 2015, 6:43 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 29
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line29>
> >
> > I think Impala may also use **HiveAuditMetadataLogEntity**, Do you think **DbAuditMetadataLogEntity** is more suitable?
thanks for the comments, HiveAuditMetadataLogEntity is only for hive, and there is a GMAuditMetadataLogEntity for generic model. All these should be DbAuditMetadataLogEntity, so I think HiveAuditMetadataLogEntity is better.
> On July 27, 2015, 6:43 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 37
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line37>
> >
> > Is there any place need constructor without field? if not, we may need to delete it or mark it private
Yes, this constructor is used in core function and test case.
> On July 27, 2015, 6:43 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 109
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line109>
> >
> > If we return the new StringWriter, will the audit log still work? If not, we may need to throw the exception.
The code here is to process the problems when write the audit log, and just output an empty audit log. I think the exception shouldn't be thrown here and log the problem is ok, because the audit log's problem shouldn't block the normal authorization process.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review93086
-----------------------------------------------------------
On July 27, 2015, 3:09 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 3:09 a.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Colin Ma <ju...@intel.com>.
> On July 27, 2015, 6:43 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java, line 109
> > <https://reviews.apache.org/r/36832/diff/1/?file=1022150#file1022150line109>
> >
> > If we return the new StringWriter, will the audit log still work? If not, we may need to throw the exception.
>
> Colin Ma wrote:
> The code here is to process the problems when write the audit log, and just output an empty audit log. I think the exception shouldn't be thrown here and log the problem is ok, because the audit log's problem shouldn't block the normal authorization process.
>
> Dapeng Sun wrote:
> I don't sure what is the best practice, +0 about issue.
I'll fix it, thanks for the comments.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review93086
-----------------------------------------------------------
On July 28, 2015, 1:14 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 1:14 a.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: Refactor the AuditMetadataLogEntity to
support the audit log for generic mdoel
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review93086
-----------------------------------------------------------
Most of the code look good, a few of comments below. Please let me know if you have any question?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java (line 29)
<https://reviews.apache.org/r/36832/#comment147346>
I think Impala may also use **HiveAuditMetadataLogEntity**, Do you think **DbAuditMetadataLogEntity** is more suitable?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java (line 37)
<https://reviews.apache.org/r/36832/#comment147344>
Is there any place need constructor without field? if not, we may need to delete it or mark it private
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java (line 109)
<https://reviews.apache.org/r/36832/#comment147347>
If we return the new StringWriter, will the audit log still work? If not, we may need to throw the exception.
- Dapeng Sun
On 七月 27, 2015, 11:09 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated 七月 27, 2015, 11:09 a.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review95825
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java (line 29)
<https://reviews.apache.org/r/36832/#comment150969>
This class name should be **TestDBAuditMetadataLogEntity**
Most looks good to me, one minor comment left
- Dapeng Sun
On 八月 13, 2015, 1:11 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated 八月 13, 2015, 1:11 p.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/DBAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review100345
-----------------------------------------------------------
Ship it!
Ship It!
- Dapeng Sun
On 八月 13, 2015, 1:11 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated 八月 13, 2015, 1:11 p.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/DBAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 5:11 a.m.)
Review request for sentry, Dapeng Sun and shen guoquan.
Repository: sentry
Description
-------
Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/DBAuditMetadataLogEntity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
Diff: https://reviews.apache.org/r/36832/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 36832: SENTRY-813: Refactor the
AuditMetadataLogEntity to support the audit log for generic mdoel
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/
-----------------------------------------------------------
(Updated July 28, 2015, 1:14 a.m.)
Review request for sentry, Dapeng Sun and shen guoquan.
Summary (updated)
-----------------
SENTRY-813: Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
Repository: sentry
Description
-------
Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
Diff: https://reviews.apache.org/r/36832/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 36832: Refactor the AuditMetadataLogEntity to
support the audit log for generic mdoel
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36832/#review93090
-----------------------------------------------------------
I think it is better to add the jira number in RB when you update your patch.
- Dapeng Sun
On 七月 27, 2015, 11:09 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36832/
> -----------------------------------------------------------
>
> (Updated 七月 27, 2015, 11:09 a.m.)
>
>
> Review request for sentry, Dapeng Sun and shen guoquan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Refactor the AuditMetadataLogEntity to support the audit log for generic mdoel
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java 6b63045
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/HiveAuditMetadataLogEntity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java 95b51e9
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestHiveAuditMetadataLogEntity.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36832/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>