mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
Date Wed, 17 Feb 2016 00:49:30 GMT


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > docs/attributes-resources.md, line 7
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252177#file1252177line7>
> >
> >     Any related to this patches?

Not really, just a minor doc cleanup I made along the way. Happy to split into a separate
patch if you'd prefer.


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > docs/attributes-resources.md, line 39
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252177#file1252177line39>
> >
> >     There seems two space before "Those are used to ..." in review board; but it's
OK drop this issues if it's one in the code.

Two spaces after a period is considered good style in English prose according to some people
(this style is used in various places throughout the comments and docs, but we aren't consistent).
I don't have a strong view, but it will require changing a lot more places than just here
to adopt a single style.


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/common/values.cpp, line 52
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252182#file1252182line52>
> >
> >     remove empty line.

I'd prefer to keep the newline: without a newline, it suggests that the comment is specific
to the function that follows (operator<<), which would be misleading: the comment applies
to the following ~7 functions.


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/common/values.cpp, line 60
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252182#file1252182line60>
> >
> >     Regarnding `lround`, it maybe overcommit resources, if framework keep launching
task with smaller CPU usage; I'd suggest to discard the additional precission directly (e.g.
1.2345 -> 1.234). If there's any idle resources because of discarding, oversubscription
will help.
> >     
> >     And we need to reject operation request with small resources which is handled
in MESOS-1807.

Truncation rather than rounding might also be reasonable behavior; I'm curious what other
people think.


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/common/values.cpp, line 61
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252182#file1252182line61>
> >
> >     Should we `setprecision`?

I didn't use setprecision, because:

1. There is value in making sure we use the same rounding method in this operator as elsewhere,
e.g, for corner-cases like 1.2345
2. setprecision is side-effecting and modifying the caller's ostream seems problematic.


> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote:
> > src/common/values.cpp, line 67
> > <https://reviews.apache.org/r/43635/diff/1/?file=1252182#file1252182line67>
> >
> >     Let's add check on overflow; it will be helpful if scalar value was big. Scalar
is a general type, there maybe used with a big value, e.g. total size of distributed filesystem.

What should we do in case of overflow?

Note that we don't check for overflow in operator+ (or for negative result values in operator-)...


- Neil


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


On Feb. 16, 2016, 11:29 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43635/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2016, 11:29 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-4687
>     https://issues.apache.org/jira/browse/MESOS-4687
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Scalar resource values are represented using floating point. As a result, users
> could see unexpected results when accepting offers and making reservations for
> fractional resources: values like "0.1" cannot be precisely represented using
> standard floating point, and the resource values returned to frameworks might
> contain an unpredictable amount of roundoff error.
> 
> This commit adjusts the master to use fixed-point when doing internal
> computations on scalar resource values. The fixed-point format only supports
> three decimal digits of precision: that is, fractional resource values like
> "0.001" will be supported, but "0.0001" will not be.
> 
> 
> Diffs
> -----
> 
>   docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e 
>   docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/master/allocator/mesos/hierarchical.cpp a9d2c23162892e22220f97d89a076d2311091d91

>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/v1/resources.cpp 207eb61d6a6d03d314539d42751cac65fcffa9af 
>   src/v1/values.cpp 58ea9875804bf0287855a1e9855855e5e54de4c4 
> 
> Diff: https://reviews.apache.org/r/43635/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Manually verified that some of the floating point oddities in https://issues.apache.org/jira/browse/MESOS-4071
do not occur when this patch is applied, although I wasn't able to reproduce the crash described
in that ticket.
> 
> REVIEW NOTES:
> 
> * We don't currently emit a warning when discarding additional digits of precision from
input scalar resource values. Should we? That would require identifying all the points where
a resource value is seemed to be "user-provided", and also runs the risk of generating a ton
of log messages when an old framework is used.
> * Similarly, if the user gives us a resource value and we don't do anything to it, we
won't discard any additional precision that appears in the value -- the precision only gets
discarded when we apply an operator like `+` or `-`. Unclear if we should trim additional
precision from all scalar resource values more aggressively.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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