mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Coffler <j...@taltos.com>
Subject Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).
Date Wed, 08 Nov 2017 00:38:56 GMT


> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> >     The general pattern is to just include the reason for an error, and to not include
any information the caller already has, because otherwise the callers will double log:
> >     
> >     ```
> >     Try<Nothing> copy = copyfile(source, destination);
> >     
> >     if (copy.isError()) {
> >       return ("Failed to copy '" + source + "'"
> >               " to '" + destination + "': " + copy.error();
> >     }
> >     ```
> >     
> >     The current code would log:
> >     
> >     "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space left"
> >     
> >     Can you guys do a sweep for this issue in the windows related code that has
been added?
> 
> Andrew Schwartzmeyer wrote:
>     I'm not sure I follow. Are you saying the _caller_ should always write the actual
error message, versus the _callee_ preparing it here?
>     
>     I guess in your example, I don't get why the user of `os::copyfile` would add `"Failed
to copy..."` instead of just doing:
>     
>     ```
>     if (copy.isError()) {
>         return Error(copy.error());
>     }
>     ```
>     
>     And then the error message is only written once, in `os::copyfile`.
> 
> Benjamin Mahler wrote:
>     What is the "actual" error message? :)
>     
>     An error message consists of several parts, much like an exception: the "reason"
for the error, and multiple "stacks" of context. If you're referring to the "reason" when
you said "actual", either approach (the one we use, or the one used in this patch) includes
the reason in their returned error message. The distinction lies in where the "stacks" of
context get included.
>     
>     The decision we took some time ago was to have the "owner" of the context be responsible
for including it. So if I call `os::copyfile` I know which function I'm calling and which
source and destination I'm passing into it. This matches posix-style programming which I why
I think we chose this approach:
>     
>     ```
>     int main()
>     {
>       int fd = open("/file");
>       
>       if (fd == -1) {
>         // Caller logs the thing it was doing, and gets the reason for the error:
>         LOG(ERROR) << "Failed to initialize: Failed to open '/file': " <<
strerror(errno);
>       }
>     }
>     
>     vs
>     
>     int main()
>     {
>       Try<int> fd = open("/file");
>       
>       if (fd.isError()) {
>         // Caller logs the thing it was doing, and gets the reason for the error:
>         LOG(ERROR) << "Failed to initialize: Failed to open '/file': " <<
fd.error();
>       }
>     }
>     ```
>     
>     Now of course, if you use the alternative approach to have the leaf include all the
information it has, then you have to compose differently:
>     
>     ```
>     int main()
>     {
>       Try<int> fd = os::open("/file");
>       
>       if (fd.isError()) {
>         // Caller knows that no additional context needs to be added because callee has
all of it.
>         LOG(ERROR) << "Failed to initialize: " << fd.error();
>       }
>     }
>     ```
>     
>     The approach we chose was to treat the error as just the "reason" (like strerror),
so if you want to add context to it you can.
>     
>     Both approaches work, but we have to pick one and apply it consistently as best we
can. In retrospect, I actually think the other approach would have been a better choice because
it fits more easily into Future::then style composition. But we would have to discuss the
change of approach and do a sweep, because most of the code follows the first pattern shown.
> 
> Andrew Schwartzmeyer wrote:
>     Ah, thank you for the detailed explanation. Unfortunately, the existing error handling
code was using the leaf approach, and we kept it consistent, but consistently wrong. We'll
have to do a sweep to fix it.
>     
>     This explanation was really good, do we have something like it documented? If not,
let's get it added to the style guide.

This is a general issue all over the place. Rather than fix that here, I think this should
be covered in a sweep of the code where we fix everything in one shot. So I'll drop this for
now, but we will plan on doing a sweep to fix up a bunch of things like this.


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review190043
-----------------------------------------------------------


On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li Li, and Michael
Park.
> 
> 
> Bugs: MESOS-6705
>     https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> -------
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>


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