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