You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Jeff Coffler <je...@taltos.com> on 2017/10/11 23:30:18 UTC

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

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

(Updated Oct. 11, 2017, 11:30 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.


Summary (updated)
-----------------

Added new stout capability: os::copyfile(source, dest).


Bugs: MESOS-6705
    https://issues.apache.org/jira/browse/MESOS-6705


Repository: mesos


Description (updated)
-------

Added new stout capability: os::copyfile(source, dest).


Diffs (updated)
-----

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

Changes: https://reviews.apache.org/r/60621/diff/2-3/


Testing
-------

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

Posted by Benjamin Mahler <bm...@apache.org>.


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

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.


- Benjamin


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


On Oct. 19, 2017, 9:32 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2017, 9:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
> 
> 
> 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
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.


> On Nov. 3, 2017, 11:29 a.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.

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.


- Andrew


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


On Oct. 19, 2017, 2:32 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2017, 2:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
> 
> 
> 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
> 
>