Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-04-04 Thread Daniel Pravat 

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


Ship it!




Ship It!

- Daniel Pravat


On April 3, 2016, 9:34 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated April 3, 2016, 9:34 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `network::connect` to use the typeful `Try` error state.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-04-03 Thread Michael Park 


> On March 26, 2016, 8:05 p.m., Daniel Pravat wrote:
> > 3rdparty/libprocess/include/process/network.hpp, line 75
> > 
> >
> > We tried to avoid axecuting too much code between ::connect 
> > WSAGetLastError() calls. 
> > Constructing the parameter for WindowsSocketError may overwrite the 
> > last error. It should be better to pass in the return from 
> > WSAGetLastError() as a parameter to the constructor, that is evaluate 
> > before the std::string parameter.
> 
> Michael Park wrote:
> Gave this some thought. I'm inclined to keep this as is.
> 
> Even if you're suggesting that we provide `::WSAGetLastError()` on every 
> callsite like:
> 
> ```
> int result = ::connect(...);
> 
> // BE AWARE!
> 
> // Even ignoring `::WSAGetLastError()` being Windows-specific.
> return ConnectError(::WSAGetLastError(), "Failed to connect to " + 
> stringify(address));
> ```
> 
> The order of evaluation of the parameters is unspecified.
> 
> Even if it were to be specified (left-to-right), we still need to be 
> mindful of the code between
> `::connect` and `ConnectError`. We would run into the same sequence of 
> code execution if someone
> were to pull out the expression and constructed a variable.
> 
> ```
> int result = ::connect(...);
> 
> // BE AWARE!
> 
> std::string message = "Failed to connect to " + stringify(address);  // 
> BE AWARE!
> return ConnectError(::WSAGetLastError(), message);
> ```
> 
> The only way we would completely solve this is if we were to say either: 
> `::WSAGetLastError()` must be
> called immediately after `::connect`, or ensure that nothing between 
> `::connect` and the call to
> `::WSAGetLastError()` can overwrite the error code. The latter is what we 
> currently do, and I don't think
> that reordering just the `::WSAGetLastError` call and the argument to 
> `ConnectError` is a big win.
> 
> Please share your ideas and solutions!
> 
> Daniel Pravat wrote:
> Exposing a constructor taking both the error code and the message should 
> be suficient. 
> 
> The convenience provided by the parameters reordering is not adding a lot 
> of value given the small number of instance where the error code is 
> interpreted.
> 
> Michael Park wrote:
> I'm not quite following. From
> > Exposing a constructor taking both the error code and the message 
> should be suficient. 
> 
> It seems like you're saying we should introduce a constructor that takes 
> an error code, something like:
> ```
> WindowsSockerError::WindowsSocketError(int code, const std::string& 
> message);
> ```
> But as I mentioned, even if we do that, the order of evaluation of the 
> arguments is unspecified.
> and even if we were to say, well it's "implementation-defined", we still 
> have to be mindful
> of the code between `::connect` and `WindowsSocketError`. You seem to be 
> supporting this argument
> with the second quote:
> > The convenience provided by the parameters reordering is not adding a 
> lot of value given the small number of instance where the error code is 
> interpreted.
> 
> so I'm a bit confused. Could you be a little more specific/concrete?
> 
> Daniel Pravat wrote:
> I was agreeing with you. We can both agree that we need both parameters 
> to the constructor (the error message for logging and the error code for the 
> execution flow). Your example from the first comment seems to imply you agree 
> with a constructor with two parameters. 
> 
> You also made a reference to the parameter reorder that may allow one 
> line return `return ConnectError(message, ::WSAGetLastError());`. However 
> given this error is returned/used only in a few places at this time, the 
> parameters can be in any order. 
> 
> The user (returning `WindowsError`) has to be aware that last error may 
> be overwritten, has to capture it ASAP and later used it to construct 
> `WindowsError`.

Ok, I've given this more thought and I'm still inclined to keep it the way it 
is.

Just to capture our discussion, you're suggesting that we just have 
`WindowsError` with
a constructor that looks like: `WindowsError::WindowsError(int code, const 
std::string& message);`,
and make the user pass `::GetLastError()` or `::WSAGetLastError()` explicitly. 
We could also make
`::GetLastError()` be the default or whatever. I'm not concerned about that.

Concretely, the following is what you’re looking for:

```cpp
int result = ::connect(...);
if (result < 0) {
  int code = ::WSAGetLastError();
  return WindowsError(code, "Failed to connect to " + stringify(address));
}
```

This ultimiately results in something like this:

(a)

```
int result = ::connect(...);
if (result < 0) {
#ifdef __WINDOWS__
  int code = ::WSAGetLastError();
  return

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-03-30 Thread Daniel Pravat 


> On March 26, 2016, 8:05 p.m., Daniel Pravat wrote:
> > 3rdparty/libprocess/include/process/network.hpp, line 75
> > 
> >
> > We tried to avoid axecuting too much code between ::connect 
> > WSAGetLastError() calls. 
> > Constructing the parameter for WindowsSocketError may overwrite the 
> > last error. It should be better to pass in the return from 
> > WSAGetLastError() as a parameter to the constructor, that is evaluate 
> > before the std::string parameter.
> 
> Michael Park wrote:
> Gave this some thought. I'm inclined to keep this as is.
> 
> Even if you're suggesting that we provide `::WSAGetLastError()` on every 
> callsite like:
> 
> ```
> int result = ::connect(...);
> 
> // BE AWARE!
> 
> // Even ignoring `::WSAGetLastError()` being Windows-specific.
> return ConnectError(::WSAGetLastError(), "Failed to connect to " + 
> stringify(address));
> ```
> 
> The order of evaluation of the parameters is unspecified.
> 
> Even if it were to be specified (left-to-right), we still need to be 
> mindful of the code between
> `::connect` and `ConnectError`. We would run into the same sequence of 
> code execution if someone
> were to pull out the expression and constructed a variable.
> 
> ```
> int result = ::connect(...);
> 
> // BE AWARE!
> 
> std::string message = "Failed to connect to " + stringify(address);  // 
> BE AWARE!
> return ConnectError(::WSAGetLastError(), message);
> ```
> 
> The only way we would completely solve this is if we were to say either: 
> `::WSAGetLastError()` must be
> called immediately after `::connect`, or ensure that nothing between 
> `::connect` and the call to
> `::WSAGetLastError()` can overwrite the error code. The latter is what we 
> currently do, and I don't think
> that reordering just the `::WSAGetLastError` call and the argument to 
> `ConnectError` is a big win.
> 
> Please share your ideas and solutions!

Exposing a constructor taking both the error code and the message should be 
suficient. 

The convenience provided by the parameters reordering is not adding a lot of 
value given the small number of instance where the error code is interpreted.


- Daniel


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


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-03-30 Thread Michael Park 


> On March 26, 2016, 8:05 p.m., Daniel Pravat wrote:
> > 3rdparty/libprocess/include/process/network.hpp, line 75
> > 
> >
> > We tried to avoid axecuting too much code between ::connect 
> > WSAGetLastError() calls. 
> > Constructing the parameter for WindowsSocketError may overwrite the 
> > last error. It should be better to pass in the return from 
> > WSAGetLastError() as a parameter to the constructor, that is evaluate 
> > before the std::string parameter.

Gave this some thought. I'm inclined to keep this as is.

Even if you're suggesting that we provide `::WSAGetLastError()` on every 
callsite like:

```
int result = ::connect(...);

// BE AWARE!

// Even ignoring `::WSAGetLastError()` being Windows-specific.
return ConnectError(::WSAGetLastError(), "Failed to connect to " + 
stringify(address));
```

The order of evaluation of the parameters is unspecified.

Even if it were to be specified (left-to-right), we still need to be mindful of 
the code between
`::connect` and `ConnectError`. We would run into the same sequence of code 
execution if someone
were to pull out the expression and constructed a variable.

```
int result = ::connect(...);

// BE AWARE!

std::string message = "Failed to connect to " + stringify(address);  // BE 
AWARE!
return ConnectError(::WSAGetLastError(), message);
```

The only way we would completely solve this is if we were to say either: 
`::WSAGetLastError()` must be
called immediately after `::connect`, or ensure that nothing between 
`::connect` and the call to
`::WSAGetLastError()` can overwrite the error code. The latter is what we 
currently do, and I don't think
that reordering just the `::WSAGetLastError` call and the argument to 
`ConnectError` is a big win.

Please share your ideas and solutions!


- Michael


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


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-03-26 Thread Daniel Pravat 

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




3rdparty/libprocess/include/process/network.hpp (line 75)


We tried to avoid axecuting too much code between ::connect 
WSAGetLastError() calls. 
Constructing the parameter for WindowsSocketError may overwrite the last 
error. It should be better to pass in the return from WSAGetLastError() as a 
parameter to the constructor, that is evaluate before the std::string parameter.


- Daniel Pravat


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-03-24 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [45310, 45311, 45312, 45313, 45314]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 24, 2016, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> ---
> 
> (Updated March 24, 2016, 8:23 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9976257d2d13316062bc95a22ab564ca0df165e5 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.

2016-03-24 Thread Michael Park 

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

Review request for mesos, Alex Clemmer and Joris Van Remoortere.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/include/process/network.hpp 
9976257d2d13316062bc95a22ab564ca0df165e5 
  3rdparty/libprocess/src/poll_socket.cpp 
6e6634b4b352e3723096521843546cf56ec6dd8b 

Diff: https://reviews.apache.org/r/45314/diff/


Testing
---

`make check`


Thanks,

Michael Park