Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benno Evers]

> On July 3, 2019, 4:07 p.m., Benjamin Mahler wrote:
> > Was the benchmark done with an optimized build?
> > Can you expand on the performance implications of this change in the 
> > description? (e.g. compare the min, 25th percentile, median, 75th 
> > percentile, max of at least 10 runs).

Sorry, I did not see this comment before committing. The benchmark above was 
done on my laptop, I think using an optimized build but I can't prove it 
anymore. I can repeat the benchmark on one of our build machines and record the 
results in some JIRA instead.


> On July 3, 2019, 4:07 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/pid.hpp
> > Lines 204 (patched)
> > 
> >
> > Hm.. hostname or host?

I opted for `host` here because that's the name that was already used in the 
PID-parsing code. However, since this is an internal field there should be no 
problem updating the name if we decide something else would be more appropriate.


- Benno


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


On June 26, 2019, 1:37 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 26, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9858
> https://issues.apache.org/jira/browse/MESOS-9858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/4/
> 
> 
> Testing
> ---
> 
> Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
> `--gtest_repeat=10`. There seems to be a slight degradation of performance on 
> average, but the benchmark runtime seems to be dominated by other factors, as 
> indicated by the large variance and presence of outliers on both sides.
> 
> 
> # Master
> 2519083.849973
> 2401163.451671
> 2322734.358626
> 2285763.292828
> 1883268.801707
> 2179436.004374
> 2071825.301714
> 2105588.897964
> 2067228.706673
> 
> 
> # Including UPID changes
> 2305142.571762
> 2194902.761484
> 2139113.533129
> 2024036.049603
> 1765327.993699
> 2172592.534808
> 2053217.096855
> 2103817.232509
> 1480080.596818
> 1808977.015807
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benjamin Mahler]

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


Ship it!




Was the benchmark done with an optimized build?
Can you expand on the performance implications of this change in the 
description? (e.g. compare the min, 25th percentile, median, 75th percentile, 
max of at least 10 runs).


3rdparty/libprocess/include/process/pid.hpp
Lines 204 (patched)


Hm.. hostname or host?


- Benjamin Mahler


On June 26, 2019, 1:37 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 26, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9858
> https://issues.apache.org/jira/browse/MESOS-9858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/4/
> 
> 
> Testing
> ---
> 
> Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
> `--gtest_repeat=10`. There seems to be a slight degradation of performance on 
> average, but the benchmark runtime seems to be dominated by other factors, as 
> indicated by the large variance and presence of outliers on both sides.
> 
> 
> # Master
> 2519083.849973
> 2401163.451671
> 2322734.358626
> 2285763.292828
> 1883268.801707
> 2179436.004374
> 2071825.301714
> 2105588.897964
> 2067228.706673
> 
> 
> # Including UPID changes
> 2305142.571762
> 2194902.761484
> 2139113.533129
> 2024036.049603
> 1765327.993699
> 2172592.534808
> 2053217.096855
> 2103817.232509
> 1480080.596818
> 1808977.015807
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benno Evers]

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

(Updated June 26, 2019, 1:37 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Changes
---

Added benchmark results.


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


Repository: mesos


Description
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/3/


Testing (updated)
---

Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
`--gtest_repeat=10`. There seems to be a slight degradation of performance on 
average, but the benchmark runtime seems to be dominated by other factors, as 
indicated by the large variance and presence of outliers on both sides.


# Master
2519083.849973
2401163.451671
2322734.358626
2285763.292828
1883268.801707
2179436.004374
2071825.301714
2105588.897964
2067228.706673


# Including UPID changes
2305142.571762
2194902.761484
2139113.533129
2024036.049603
1765327.993699
2172592.534808
2053217.096855
2103817.232509
1480080.596818
1808977.015807


Thanks,

Benno Evers

Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benno Evers]

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

(Updated June 24, 2019, 1:48 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


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


Repository: mesos


Description
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/3/

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benno Evers]

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




3rdparty/libprocess/src/pid.cpp
Lines 74 (patched)


Note: I'm a bit unsure about this. On the one hand, it feels super-useful 
to print UPID's the same way they were received, and I've been annoyed more 
often than not that I have to manually resolve an IP address from some Mesos 
log.

On the other hand, this can lead to slightly confusing messages like:
```
W0622 00:09:43.911963  5825 slave.cpp:1584] Ignoring re-registration 
message from master@127.0.1.1:5050 because it is not the expected master: 
master@localhost:5050

```
where it's not immediately obvious that `localhost` is actually ignored by 
the code in question and the problem is actually that it resolved to 
`127.0.0.1` and not `127.0.1.1`.


- Benno Evers


On June 21, 2019, 10:25 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 21, 2019, 10:25 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
>   3rdparty/libprocess/src/process.cpp 
> 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70884: Added optional 'host' string member to UPID.

[Benno Evers]

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

(Updated June 21, 2019, 10:25 p.m.)


Review request for mesos, Joseph Wu and Till Toenshoff.


Summary (updated)
-

Added optional 'host' string member to UPID.


Repository: mesos


Description (updated)
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/2/

Changes: https://reviews.apache.org/r/70884/diff/1-2/


Testing
---


Thanks,

Benno Evers