mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 37584: Fix bug accessing error() when no Error
Date Tue, 18 Aug 2015 19:26:46 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message