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