You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/09/22 08:09:33 UTC
Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Bugs: SENTRY-1963
https://issues.apache.org/jira/browse/SENTRY-1963
Repository: sentry
Description
-------
SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryMetrics.java PRE-CREATION
Diff: https://reviews.apache.org/r/62496/diff/1/
Testing
-------
New unit test written
Thanks,
Alexander Kolbasov
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review187005
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Oct. 2, 2017, 3:11 p.m., kalyan kumar kalvagadda wrote:
> > Sasha, Could you please explain "what does regular implementation for local file system" mean? That will help get the conetxt what you are trying to do.
The original implementation uses Hadoop local filesystem interface which isn't needed here at all - we are much better off using java.nio file system operations instead.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review186848
-----------------------------------------------------------
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review186848
-----------------------------------------------------------
Sasha, Could you please explain "what does regular implementation for local file system" mean? That will help get the conetxt what you are trying to do.
- kalyan kumar kalvagadda
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review187001
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
Lines 400 (patched)
<https://reviews.apache.org/r/62496/#comment263952>
Is it the right format? Per javadoc, it would matche this signature: "error(String format, Object arg1, Object arg2)", where exception e is just another object, so there should be a placeholder for it in a format string. The signature for Throwable is "error(String msg, Throwable t)", which would require "LOGGER.error("Unable to delete " + path, e);"
- Vadim Spector
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review186021
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review187006
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/
-----------------------------------------------------------
(Updated Sept. 22, 2017, 6:28 p.m.)
Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Addressed code review comment from Lina plus more comments
Bugs: SENTRY-1963
https://issues.apache.org/jira/browse/SENTRY-1963
Repository: sentry
Description
-------
SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
Diff: https://reviews.apache.org/r/62496/diff/3/
Changes: https://reviews.apache.org/r/62496/diff/2-3/
Testing
-------
New unit test written
Thanks,
Alexander Kolbasov
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Sept. 22, 2017, 3:55 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
> > Line 319 (original), 337 (patched)
> > <https://reviews.apache.org/r/62496/diff/2/?file=1832780#file1832780line346>
> >
> > This is similar to "tmpFile = Files.createTempFile(tmpDir, "hmetrics", "json", FILE_ATTRS);" in https://reviews.apache.org/r/62487/diff/2#0
> >
> > This means both temporary file has the same name. Is the temDir of the same value?
> >
> > SENTRY_JSON_REPORTER_FILE_DEFAULT = "/tmp/sentry-metrics.json";
> >
> > This means the temDir is <log home>/tmp, so sentry temp metric file will be of the same name with hive temp metric file. Is this correct?
> >
> > Can we use the temFile name defined in ServerConfig.SENTRY_JSON_REPORTER_FILE and ServerConfig.SENTRY_JSON_REPORTER_FILE_DEFAULT?
createTempFile creates a file with unique name every time it is called. but we can change prefix as well to be clear what this file is.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review185997
-----------------------------------------------------------
On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 6:28 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
>
>
> Diff: https://reviews.apache.org/r/62496/diff/3/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/#review185997
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
Line 319 (original), 337 (patched)
<https://reviews.apache.org/r/62496/#comment262341>
This is similar to "tmpFile = Files.createTempFile(tmpDir, "hmetrics", "json", FILE_ATTRS);" in https://reviews.apache.org/r/62487/diff/2#0
This means both temporary file has the same name. Is the temDir of the same value?
SENTRY_JSON_REPORTER_FILE_DEFAULT = "/tmp/sentry-metrics.json";
This means the temDir is <log home>/tmp, so sentry temp metric file will be of the same name with hive temp metric file. Is this correct?
Can we use the temFile name defined in ServerConfig.SENTRY_JSON_REPORTER_FILE and ServerConfig.SENTRY_JSON_REPORTER_FILE_DEFAULT?
- Na Li
On Sept. 22, 2017, 8:19 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 8:19 a.m.)
>
>
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryMetrics.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62496/diff/2/
>
>
> Testing
> -------
>
> New unit test written
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use
regular implementation for local file system
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62496/
-----------------------------------------------------------
(Updated Sept. 22, 2017, 8:19 a.m.)
Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Removed unused imports
Bugs: SENTRY-1963
https://issues.apache.org/jira/browse/SENTRY-1963
Repository: sentry
Description
-------
SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java 4063a663c86ea5ed88c40013d5b3550ac0a6272f
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryMetrics.java PRE-CREATION
Diff: https://reviews.apache.org/r/62496/diff/2/
Changes: https://reviews.apache.org/r/62496/diff/1-2/
Testing
-------
New unit test written
Thanks,
Alexander Kolbasov