You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jared Stewart <js...@pivotal.io> on 2017/05/04 17:12:43 UTC
Review Request 58996: GEODE-2876: Add logging to diagnose CI failure
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/
-----------------------------------------------------------
Review request for geode.
Repository: geode
Description
-------
GEODE-2876: Add logging to diagnose CI failure
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68
Diff: https://reviews.apache.org/r/58996/diff/1/
Testing
-------
Thanks,
Jared Stewart
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review174200
-----------------------------------------------------------
Ship it!
Ship It!
- Jinmei Liao
On May 5, 2017, 11:15 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 11:15 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 20ae022
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e14
>
>
> Diff: https://reviews.apache.org/r/58996/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review174197
-----------------------------------------------------------
Ship it!
Ship It!
- Patrick Rhomberg
On May 5, 2017, 11:15 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 11:15 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 20ae022
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e14
>
>
> Diff: https://reviews.apache.org/r/58996/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review174201
-----------------------------------------------------------
Ship it!
Ship It!
- Ken Howe
On May 5, 2017, 11:15 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 11:15 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 20ae022
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e14
>
>
> Diff: https://reviews.apache.org/r/58996/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/
-----------------------------------------------------------
(Updated May 5, 2017, 11:15 p.m.)
Review request for geode.
Repository: geode
Description
-------
GEODE-2876: Add logging to diagnose CI failure
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 20ae022
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e14
Diff: https://reviews.apache.org/r/58996/diff/2/
Changes: https://reviews.apache.org/r/58996/diff/1-2/
Testing
-------
Thanks,
Jared Stewart
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review173938
-----------------------------------------------------------
Ship it!
Ship It!
- Kirk Lund
On May 4, 2017, 5:12 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 5:12 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68
>
>
> Diff: https://reviews.apache.org/r/58996/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Jinmei Liao <ji...@pivotal.io>.
> On May 4, 2017, 5:29 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
> > Line 124 (original), 125 (patched)
> > <https://reviews.apache.org/r/58996/diff/1/?file=1708729#file1708729line125>
> >
> > we are not showing info level logging in our current logs?
> >
> > Also the best place to see what's being actually parsed is in GfshParser.parse method. That's where we are handing the input string to SpringShell's parsing method.
>
> Jared Stewart wrote:
> I think this really does belong at "warning" level. (If a user tries to execute a command that can't be parsed, I think that should still be logged even if they have set the log-level to WARN.) That said, I'm a bit puzzled by the current behavior. The log statement does show up in the log, but it appears to be missing the accompanying stack trace.
> ```
> [vm3] Command result for <deploy --jar=/tmp/junit3108566533782045832/jar1.jar>:
> [vm3] Could not parse command string. deploy --jar=/tmp/junit3108566533782045832/jar1.jar
> ```
>
> The other change should fix this by passing the full stacktrace back to the client in the gfsh ErrorResult. (I think this would also make it easier for a real user either to diagnose their failure or to file a descriptive bug report.)
when failed to parse, there is no stacktrace thrown by Spring's SimpleParser, it just simply returns null. That's why I would like to get a log statement right before we pass the string to Spring's parser and see what it failed to parse. then we can write a simple unit tests to find out.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review173930
-----------------------------------------------------------
On May 4, 2017, 5:12 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 5:12 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68
>
>
> Diff: https://reviews.apache.org/r/58996/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Jared Stewart <js...@pivotal.io>.
> On May 4, 2017, 5:29 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
> > Line 124 (original), 125 (patched)
> > <https://reviews.apache.org/r/58996/diff/1/?file=1708729#file1708729line125>
> >
> > we are not showing info level logging in our current logs?
> >
> > Also the best place to see what's being actually parsed is in GfshParser.parse method. That's where we are handing the input string to SpringShell's parsing method.
I think this really does belong at "warning" level. (If a user tries to execute a command that can't be parsed, I think that should still be logged even if they have set the log-level to WARN.) That said, I'm a bit puzzled by the current behavior. The log statement does show up in the log, but it appears to be missing the accompanying stack trace.
```
[vm3] Command result for <deploy --jar=/tmp/junit3108566533782045832/jar1.jar>:
[vm3] Could not parse command string. deploy --jar=/tmp/junit3108566533782045832/jar1.jar
```
The other change should fix this by passing the full stacktrace back to the client in the gfsh ErrorResult. (I think this would also make it easier for a real user either to diagnose their failure or to file a descriptive bug report.)
- Jared
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review173930
-----------------------------------------------------------
On May 4, 2017, 5:12 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 5:12 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68
>
>
> Diff: https://reviews.apache.org/r/58996/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>
Re: Review Request 58996: GEODE-2876: Add logging to diagnose CI
failure
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58996/#review173930
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
Line 124 (original), 125 (patched)
<https://reviews.apache.org/r/58996/#comment247005>
we are not showing info level logging in our current logs?
Also the best place to see what's being actually parsed is in GfshParser.parse method. That's where we are handing the input string to SpringShell's parsing method.
- Jinmei Liao
On May 4, 2017, 5:12 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58996/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 5:12 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2876: Add logging to diagnose CI failure
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68
>
>
> Diff: https://reviews.apache.org/r/58996/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jared Stewart
>
>