> 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
> 
>

Reply via email to