You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/08/18 19:24:38 UTC
Review Request 37584: Fix bug accessing error() when no Error
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37584/
-----------------------------------------------------------
Review request for mesos and Adam B.
Bugs: MESOS-3287
https://issues.apache.org/jira/browse/MESOS-3287
Repository: mesos
Description
-------
When trying to download an artifact with the Hadoop client,
and the client is not `available()` we correctly return a
false value, but then in the error message, we tried to
make a call to `Try::error` which failed and crashed Master.
This fixes it.
Diffs
-----
src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3
Diff: https://reviews.apache.org/r/37584/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 37584: Fix bug accessing error() when no Error
Posted by Vinod Kone <vi...@gmail.com>.
> On Aug. 18, 2015, 6:34 p.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, lines 100-106
> > <https://reviews.apache.org/r/37584/diff/1/?file=1043203#file1043203line100>
> >
> > just do
> >
> > return Error("Skipping fetch with Hadoop client: " +
> > (available.isError() ? availabe.error() : " client not found"));
>
> Marco Massenzio wrote:
> So, that was my first choice, but I then reflected that my finding out the site of the error was made more complicated due to the available log lines being far away from it; so I had to do some digging and investigating.
>
> In general, I have been taught to prefer to have a LOG(ERROR) near the site where the error actually happens, as that also avoids running wild goose chases :)
> Especially if it so happens that the error message may be (unintentionally) misleading.
>
> What do you think?
I think it's not consistent with how other cases are handled in this file. This particular should've been easy to find because the call site is also in fetcher.cpp. I'll fix this and commit for you.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37584/#review95754
-----------------------------------------------------------
On Aug. 18, 2015, 5:24 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37584/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2015, 5:24 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-3287
> https://issues.apache.org/jira/browse/MESOS-3287
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When trying to download an artifact with the Hadoop client,
> and the client is not `available()` we correctly return a
> false value, but then in the error message, we tried to
> make a call to `Try::error` which failed and crashed Master.
>
> This fixes it.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3
>
> Diff: https://reviews.apache.org/r/37584/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 37584: Fix bug accessing error() when no Error
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Aug. 18, 2015, 6:34 p.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, lines 100-106
> > <https://reviews.apache.org/r/37584/diff/1/?file=1043203#file1043203line100>
> >
> > just do
> >
> > return Error("Skipping fetch with Hadoop client: " +
> > (available.isError() ? availabe.error() : " client not found"));
So, that was my first choice, but I then reflected that my finding out the site of the error was made more complicated due to the available log lines being far away from it; so I had to do some digging and investigating.
In general, I have been taught to prefer to have a LOG(ERROR) near the site where the error actually happens, as that also avoids running wild goose chases :)
Especially if it so happens that the error message may be (unintentionally) misleading.
What do you think?
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37584/#review95754
-----------------------------------------------------------
On Aug. 18, 2015, 5:24 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37584/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2015, 5:24 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-3287
> https://issues.apache.org/jira/browse/MESOS-3287
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When trying to download an artifact with the Hadoop client,
> and the client is not `available()` we correctly return a
> false value, but then in the error message, we tried to
> make a call to `Try::error` which failed and crashed Master.
>
> This fixes it.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3
>
> Diff: https://reviews.apache.org/r/37584/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 37584: Fix bug accessing error() when no Error
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 18, 2015, 6:34 p.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, lines 100-106
> > <https://reviews.apache.org/r/37584/diff/1/?file=1043203#file1043203line100>
> >
> > just do
> >
> > return Error("Skipping fetch with Hadoop client: " +
> > (available.isError() ? availabe.error() : " client not found"));
>
> Marco Massenzio wrote:
> So, that was my first choice, but I then reflected that my finding out the site of the error was made more complicated due to the available log lines being far away from it; so I had to do some digging and investigating.
>
> In general, I have been taught to prefer to have a LOG(ERROR) near the site where the error actually happens, as that also avoids running wild goose chases :)
> Especially if it so happens that the error message may be (unintentionally) misleading.
>
> What do you think?
>
> Vinod Kone wrote:
> I think it's not consistent with how other cases are handled in this file. This particular should've been easy to find because the call site is also in fetcher.cpp. I'll fix this and commit for you.
Well, that leads to double logging unfortunately, since both the callee and caller log the same error, no? Typically if we have a library function that returns a Try, we'll put enough information in the Try error, and leave it up to the caller to decide how to log it. Otherwise, logging gets a bit hard to manage, make sense?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37584/#review95754
-----------------------------------------------------------
On Aug. 18, 2015, 5:24 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37584/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2015, 5:24 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-3287
> https://issues.apache.org/jira/browse/MESOS-3287
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When trying to download an artifact with the Hadoop client,
> and the client is not `available()` we correctly return a
> false value, but then in the error message, we tried to
> make a call to `Try::error` which failed and crashed Master.
>
> This fixes it.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3
>
> Diff: https://reviews.apache.org/r/37584/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 37584: Fix bug accessing error() when no Error
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37584/#review95754
-----------------------------------------------------------
src/launcher/fetcher.cpp (lines 100 - 106)
<https://reviews.apache.org/r/37584/#comment150866>
just do
return Error("Skipping fetch with Hadoop client: " +
(available.isError() ? availabe.error() : " client not found"));
- Vinod Kone
On Aug. 18, 2015, 5:24 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37584/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2015, 5:24 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-3287
> https://issues.apache.org/jira/browse/MESOS-3287
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When trying to download an artifact with the Hadoop client,
> and the client is not `available()` we correctly return a
> false value, but then in the error message, we tried to
> make a call to `Try::error` which failed and crashed Master.
>
> This fixes it.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp f8b46b8f957942d0cfdc45d3361f52dae3e514a3
>
> Diff: https://reviews.apache.org/r/37584/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>