You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Naoki Takezoe <ta...@apache.org> on 2021/06/15 21:46:43 UTC
Review Request 73424: RANGER-3221: Improve logging in Presto plugin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/
-----------------------------------------------------------
Review request for ranger.
Repository: ranger
Description
-------
Would make logging in Presto plugin more consistent.
Diffs
-----
plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
Diff: https://reviews.apache.org/r/73424/diff/1/
Testing
-------
Thanks,
Naoki Takezoe
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Naoki Takezoe <ta...@apache.org>.
> On June 16, 2021, 6:33 a.m., Ramesh Mani wrote:
> > Ship It!
Hmm... Looks like this hasn't been applied yet. Was my patch wrong again?
- Naoki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223160
-----------------------------------------------------------
On June 16, 2021, 6:09 a.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 16, 2021, 6:09 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Ramesh Mani <rm...@hortonworks.com>.
> On June 16, 2021, 6:33 a.m., Ramesh Mani wrote:
> > Ship It!
>
> Naoki Takezoe wrote:
> Hmm... Looks like this hasn't been applied yet. Was my patch wrong again?
Patch applied to master branch. Thanks!
- Ramesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223160
-----------------------------------------------------------
On June 16, 2021, 6:09 a.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 16, 2021, 6:09 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Ramesh Mani <rm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223160
-----------------------------------------------------------
Ship it!
Ship It!
- Ramesh Mani
On June 16, 2021, 6:09 a.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 16, 2021, 6:09 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Naoki Takezoe <ta...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/
-----------------------------------------------------------
(Updated June 16, 2021, 6:09 a.m.)
Review request for ranger.
Changes
-------
slightly updated for further fixes
Repository: ranger
Description
-------
Would make logging in Presto plugin more consistent.
Diffs (updated)
-----
plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
Diff: https://reviews.apache.org/r/73424/diff/2/
Changes: https://reviews.apache.org/r/73424/diff/1-2/
Testing
-------
Thanks,
Naoki Takezoe
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Naoki Takezoe <ta...@apache.org>.
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line109>
> >
> > <== instead of ==>
>
> Naoki Takezoe wrote:
> Also, this looks entering a branch. Why should `<==` (= exit) be used here? Exit from what?
>
> Looks like there are many inconsistent logging even in Presto plugin. Could you give more explanation about logging regulation so that I can fix all logging in Presto plugin in this review request.
For example, some methods have multiple `==>` in a single method. Some methods have `<==` with error level not debug level. It would be impossible to correct all ocureences without having solid regulation. I just aim to correct wrong wordings in Presto plugin for searchability of logs in this review request.
- Naoki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223153
-----------------------------------------------------------
On June 15, 2021, 9:46 p.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 15, 2021, 9:46 p.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Naoki Takezoe <ta...@apache.org>.
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line109>
> >
> > <== instead of ==>
>
> Naoki Takezoe wrote:
> Also, this looks entering a branch. Why should `<==` (= exit) be used here? Exit from what?
>
> Looks like there are many inconsistent logging even in Presto plugin. Could you give more explanation about logging regulation so that I can fix all logging in Presto plugin in this review request.
>
> Naoki Takezoe wrote:
> For example, some methods have multiple `==>` in a single method. Some methods have `<==` with error level not debug level. It would be impossible to correct all ocureences without having solid regulation. I just aim to correct wrong wordings in Presto plugin for searchability of logs in this review request.
>
> Ramesh Mani wrote:
> ok then, I see that presto plugin have inconsistent usage of "==>". We can omit that change for this patch and can be corrected later.
Thank you!
- Naoki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223153
-----------------------------------------------------------
On June 16, 2021, 6:09 a.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 16, 2021, 6:09 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Naoki Takezoe <ta...@apache.org>.
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 44 (original), 44 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line44>
> >
> > Error message need not to have "==>". Symbol "==>/<==" is to show in debug log entry/exit of the routine. please consider correcting it all off this occurance.
Hmm... Isn't this a debug log for entering `connectionTest`? A routine doesn't mean a method? I'm confused...
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line109>
> >
> > <== instead of ==>
Also, this looks entering a branch. Why should `<==` (= exit) be used here? Exit from what?
Looks like there are many inconsistent logging even in Presto plugin. Could you give more explanation about logging regulation so that I can fix all logging in Presto plugin in this review request.
- Naoki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223153
-----------------------------------------------------------
On June 15, 2021, 9:46 p.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 15, 2021, 9:46 p.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Ramesh Mani <rm...@hortonworks.com>.
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 44 (original), 44 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line44>
> >
> > Error message need not to have "==>". Symbol "==>/<==" is to show in debug log entry/exit of the routine. please consider correcting it all off this occurance.
>
> Naoki Takezoe wrote:
> Hmm... Isn't this a debug log for entering `connectionTest`? A routine doesn't mean a method? I'm confused...
I meant the method. ==> and <== used for priting debug logs with details of parameters and return values. If the right symbol is used that is fine then.
> On June 15, 2021, 9:59 p.m., Ramesh Mani wrote:
> > plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/73424/diff/1/?file=2250920#file2250920line109>
> >
> > <== instead of ==>
>
> Naoki Takezoe wrote:
> Also, this looks entering a branch. Why should `<==` (= exit) be used here? Exit from what?
>
> Looks like there are many inconsistent logging even in Presto plugin. Could you give more explanation about logging regulation so that I can fix all logging in Presto plugin in this review request.
>
> Naoki Takezoe wrote:
> For example, some methods have multiple `==>` in a single method. Some methods have `<==` with error level not debug level. It would be impossible to correct all ocureences without having solid regulation. I just aim to correct wrong wordings in Presto plugin for searchability of logs in this review request.
ok then, I see that presto plugin have inconsistent usage of "==>". We can omit that change for this patch and can be corrected later.
- Ramesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223153
-----------------------------------------------------------
On June 16, 2021, 6:09 a.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 16, 2021, 6:09 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>
Re: Review Request 73424: RANGER-3221: Improve logging in Presto
plugin
Posted by Ramesh Mani <rm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73424/#review223153
-----------------------------------------------------------
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
Line 44 (original), 44 (patched)
<https://reviews.apache.org/r/73424/#comment312259>
Error message need not to have "==>". Symbol "==>/<==" is to show in debug log entry/exit of the routine. please consider correcting it all off this occurance.
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java
Line 109 (original), 109 (patched)
<https://reviews.apache.org/r/73424/#comment312258>
<== instead of ==>
- Ramesh Mani
On June 15, 2021, 9:46 p.m., Naoki Takezoe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73424/
> -----------------------------------------------------------
>
> (Updated June 15, 2021, 9:46 p.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Would make logging in Presto plugin more consistent.
>
>
> Diffs
> -----
>
> plugin-presto/src/main/java/org/apache/ranger/services/presto/RangerServicePresto.java d95876a7d
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java 7b55b77bd
> plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoResourceManager.java 008bf0fa6
>
>
> Diff: https://reviews.apache.org/r/73424/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Naoki Takezoe
>
>