Re: Review Request 43583: Added documentation for multiple-disk support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43583/#review119357 --- Fix it, then Ship it! docs/multiple-disk.md (line 83) <https://reviews.apache.org/r/43583/#comment180673> "write-ahead logs" docs/multiple-disk.md (line 87) <https://reviews.apache.org/r/43583/#comment180674> "mount a physical disk" seems a bit better, since we talk about a single mount point later in the sentence. docs/multiple-disk.md (line 92) <https://reviews.apache.org/r/43583/#comment180671> "over-run" => "exceed" docs/multiple-disk.md (line 93) <https://reviews.apache.org/r/43583/#comment180672> "file-system mounted" => "file system in use" docs/multiple-disk.md (line 125) <https://reviews.apache.org/r/43583/#comment180675> I'd say "destroyed" w/o backticks. docs/multiple-disk.md (line 127) <https://reviews.apache.org/r/43583/#comment180676> "destroying" "strongly encouraged" docs/multiple-disk.md (line 128) <https://reviews.apache.org/r/43583/#comment180677> "do not get penalized" => "are not penalized" - Neil Conway On Feb. 16, 2016, 1:58 p.m., Joris Van Remoortere wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43583/ > --- > > (Updated Feb. 16, 2016, 1:58 p.m.) > > > Review request for mesos, Jie Yu and Neil Conway. > > > Bugs: MESOS-4531 > https://issues.apache.org/jira/browse/MESOS-4531 > > > Repository: mesos > > > Description > --- > > Added documentation for multiple-disk support. > > > Diffs > - > > docs/home.md a2000a35a6eeaa7b36cb1796532263f5a703ac88 > docs/multiple-disk.md PRE-CREATION > docs/persistent-volume.md 4d7821fc4a18ab3c6261418fb8062e6bdf90d5a3 > > Diff: https://reviews.apache.org/r/43583/diff/ > > > Testing > --- > > > Thanks, > > Joris Van Remoortere > >
Re: Review Request 43583: Added documentation for multiple-disk support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43583/#review119363 --- docs/multiple-disk.md (line 138) <https://reviews.apache.org/r/43583/#comment180680> This sentence is a bit vague/confusing. - Neil Conway On Feb. 16, 2016, 9:43 p.m., Joris Van Remoortere wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43583/ > --- > > (Updated Feb. 16, 2016, 9:43 p.m.) > > > Review request for mesos, Jie Yu and Neil Conway. > > > Bugs: MESOS-4531 > https://issues.apache.org/jira/browse/MESOS-4531 > > > Repository: mesos > > > Description > --- > > Added documentation for multiple-disk support. > > > Diffs > - > > docs/home.md a2000a35a6eeaa7b36cb1796532263f5a703ac88 > docs/multiple-disk.md PRE-CREATION > docs/persistent-volume.md 4d7821fc4a18ab3c6261418fb8062e6bdf90d5a3 > > Diff: https://reviews.apache.org/r/43583/diff/ > > > Testing > --- > > > Thanks, > > Joris Van Remoortere > >
Re: Review Request 43633: Improved Multiple Disk documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43633/#review119370 --- Ship it! Ship It! - Neil Conway On Feb. 16, 2016, 10:48 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43633/ > --- > > (Updated Feb. 16, 2016, 10:48 p.m.) > > > Review request for mesos, Joris Van Remoortere and Neil Conway. > > > Bugs: MESOS-4531 > https://issues.apache.org/jira/browse/MESOS-4531 > > > Repository: mesos > > > Description > --- > > Improved Multiple Disk documentation. > > > Diffs > - > > docs/multiple-disk.md 4fc5327f131fe571357b93569e167be0c858913e > docs/persistent-volume.md 728854076595a0908a17d85482a7031acfbb09a0 > > Diff: https://reviews.apache.org/r/43633/diff/ > > > Testing > --- > > viewed via docker website container. > > > Thanks, > > Joerg Schad > >
Re: Review Request 43634: Consistent markdown code style in persistent-volumes.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43634/#review119379 --- Ship it! Ship It! - Neil Conway On Feb. 16, 2016, 11:03 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43634/ > --- > > (Updated Feb. 16, 2016, 11:03 p.m.) > > > Review request for mesos, Michael Park and Neil Conway. > > > Repository: mesos > > > Description > --- > > Consistent markdown code style in persistent-volumes.md. > > > Diffs > - > > docs/persistent-volume.md 728854076595a0908a17d85482a7031acfbb09a0 > > Diff: https://reviews.apache.org/r/43634/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- Review request for mesos and Joris Van Remoortere. 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 a9d2c23162892e0f97d89a076d2311091d91 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
Review Request 43636: Cleaned up various code in a test file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43636/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- Cleaned up various code in a test file. Diffs - src/tests/teardown_tests.cpp 5753559003d703138d2bbee6a1ac93473ba0b0c0 Diff: https://reviews.apache.org/r/43636/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review119393 --- docs/attributes-resources.md (line 7) <https://reviews.apache.org/r/43635/#comment180733> Not really, just a minor doc cleanup I made along the way. Happy to split into a separate patch if you'd prefer. docs/attributes-resources.md (line 39) <https://reviews.apache.org/r/43635/#comment180734> 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. src/common/values.cpp (line 52) <https://reviews.apache.org/r/43635/#comment180732> 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. src/common/values.cpp (line 60) <https://reviews.apache.org/r/43635/#comment180737> Truncation rather than rounding might also be reasonable behavior; I'm curious what other people think. src/common/values.cpp (line 61) <https://reviews.apache.org/r/43635/#comment180736> 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 a side-effecting operation, so modifying the caller's `ostream` seems problematic. src/common/values.cpp (line 67) <https://reviews.apache.org/r/43635/#comment180735> 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 Conway 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 > a9d2c23162892e0f97d89a076d2311091d91 > 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. > * Similarl
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 17, 2016, 12:49 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address some of Klaus' comments. 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 (updated) - 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 a9d2c23162892e0f97d89a076d2311091d91 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
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
> 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
Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.
> On Feb. 17, 2016, 1:57 a.m., Guangya Liu wrote: > > include/mesos/mesos.proto, line 598 > > <https://reviews.apache.org/r/43616/diff/1/?file=1251896#file1251896line598> > > > > s/Labels/One label? > > > > I think that here we should use `one label` but not `labels`, becauase > > this can only happen if one label include duplicate key-value pairs. Among all the key-value pairs in a `Labels`, no two pairs should be the same. So I think the current text expresses that better than talking about an individual label. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43616/#review119399 --- On Feb. 16, 2016, 7:15 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43616/ > --- > > (Updated Feb. 16, 2016, 7:15 p.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > --- > > The implementation of the equality operator for `Labels` is buggy for labels > that contain duplicates. We might want to revisit fixing the implementation of > that operator (which might be expensive; MESOS-4445), but in the short-term we > should document that duplicates should not be specified. > > > Diffs > - > > include/mesos/mesos.proto 0bd5abadb5abe052161963ca995c396f1ed832f2 > include/mesos/v1/mesos.proto 38e04cb19e303d1c71d2afad6ea73137aaa7403a > > Diff: https://reviews.apache.org/r/43616/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43616/ --- (Updated Feb. 17, 2016, 6:44 p.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- The implementation of the equality operator for `Labels` is buggy for labels that contain duplicates. We might want to revisit fixing the implementation of that operator (which might be expensive; MESOS-4445), but in the short-term we should document that duplicates should not be specified. Diffs (updated) - include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 Diff: https://reviews.apache.org/r/43616/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
> 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. > > Neil Conway wrote: > 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-)... > > Klaus Ma wrote: > `CHECK` or warning log should be fine, it only improves the debugability; > it different with `operator+` because `max_double/2` should be big enough for > us; but for `double * 1000 -> long`, I'm not sure of that. > > And I think we need to use `long long` or `int64` instead of `long`; > `long` is not garantee to be 64bit in c++. re: `long`, good point -- we only officially support 64-bit OSX, Linux, and Windows, but `long` is 32-bit on Windows. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review119382 --- On Feb. 17, 2016, 12:49 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43635/ > --- > > (Updated Feb. 17, 2016, 12:49 a.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 > a9d2c23162892e0f97d89a076d2311091d91 > 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 > >
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 17, 2016, 6:49 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address some more code review comments. 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 (updated) - 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 a9d2c23162892e0f97d89a076d2311091d91 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
Review Request 43684: Cleaned up allocator benchmark code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43684/ --- Review request for mesos, Joris Van Remoortere and Michael Park. Repository: mesos Description --- Cleaned up allocator benchmark code. Diffs - src/tests/hierarchical_allocator_tests.cpp 990f3723d52dfeaa19d5eb0603c0fc7eb2b362c7 Diff: https://reviews.apache.org/r/43684/diff/ Testing --- make check Thanks, Neil Conway
Review Request 43685: Refactored test helper code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43685/ --- Review request for mesos, Joris Van Remoortere and Michael Park. Repository: mesos Description --- Refactored test helper code. Diffs - src/tests/mesos.hpp 242a11658c0a9ba4caced9b2b2bdbcb921f7fdd0 src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff Diff: https://reviews.apache.org/r/43685/diff/ Testing --- make check Thanks, Neil Conway
Review Request 43686: Added a benchmark for allocation with labeled resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43686/ --- Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-4691 https://issues.apache.org/jira/browse/MESOS-4691 Repository: mesos Description --- For the particular workload exercised by the benchmark, this suggests that adding a 12-element label to a resource slows down allocation by about 5% on my local machine. Diffs - src/tests/hierarchical_allocator_tests.cpp 990f3723d52dfeaa19d5eb0603c0fc7eb2b362c7 Diff: https://reviews.apache.org/r/43686/diff/ Testing --- make check FYI, results on my laptop: _Original benchmark (unlabeled resources)_ [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.028175secs to make 200 offers round 1 allocate took 2.006791secs to make 200 offers round 2 allocate took 2.033723secs to make 200 offers round 3 allocate took 2.017508secs to make 200 offers round 4 allocate took 2.037235secs to make 200 offers round 5 allocate took 2.054095secs to make 200 offers round 6 allocate took 2.048884secs to make 200 offers round 7 allocate took 2.044252secs to make 200 offers round 8 allocate took 2.060256secs to make 200 offers round 9 allocate took 2.07121secs to make 200 offers round 10 allocate took 2.066261secs to make 200 offers round 11 allocate took 2.034805secs to make 200 offers round 12 allocate took 2.053705secs to make 200 offers round 13 allocate took 2.042106secs to make 200 offers round 14 allocate took 2.082704secs to make 200 offers _New benchmark (two labeled resources with different labels)_ [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.128709secs to make 200 offers round 1 allocate took 2.188029secs to make 200 offers round 2 allocate took 2.145937secs to make 200 offers round 3 allocate took 2.171442secs to make 200 offers round 4 allocate took 2.153106secs to make 200 offers round 5 allocate took 2.151484secs to make 200 offers round 6 allocate took 2.136182secs to make 200 offers round 7 allocate took 2.152105secs to make 200 offers round 8 allocate took 2.187842secs to make 200 offers round 9 allocate took 2.13839secs to make 200 offers round 10 allocate took 2.237216secs to make 200 offers round 11 allocate took 2.164702secs to make 200 offers round 12 allocate took 2.143296secs to make 200 offers round 13 allocate took 2.198839secs to make 200 offers round 14 allocate took 2.179931secs to make 200 offers For fun, I tried running the benchmark with the modified equality operator for `Labels` that uses `unordered_multiset` to produce the correct results for labels that contain duplicates: [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.190051secs to make 200 offers round 1 allocate took 2.169332secs to make 200 offers round 2 allocate took 2.156235secs to make 200 offers round 3 allocate took 2.15506secs to make 200 offers round 4 allocate took 2.133953secs to make 200 offers round 5 allocate took 2.18325secs to make 200 offers round 6 allocate took 2.164478secs to make 200 offers round 7 allocate took 2.192077secs to make 200 offers round 8 allocate took 2.14688secs to make 200 offers round 9 allocate took 2.172333secs to make 200 offers round 10 allocate took 2.199906secs to make 200 offers round 11 allocate took 2.16384secs to make 200 offers round 12 allocate took 2.200181secs to make 200 offers round 13 allocate took 2.138463secs to make 200 offers round 14 allocate took 2.184699secs to make 200 offers Thanks, Neil Conway
Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.
> On Feb. 17, 2016, 1:57 a.m., Guangya Liu wrote: > > include/mesos/mesos.proto, line 598 > > <https://reviews.apache.org/r/43616/diff/1/?file=1251896#file1251896line598> > > > > s/Labels/One label? > > > > I think that here we should use `one label` but not `labels`, becauase > > this can only happen if one label include duplicate key-value pairs. > > Neil Conway wrote: > Among all the key-value pairs in a `Labels`, no two pairs should be the > same. So I think the current text expresses that better than talking about an > individual label. > > Guangya Liu wrote: > May be we need to be more accurate, if `Labels should not contain > duplicate key-value pairs`, then how can mesos define same labels? "Among all the key-value pairs in a Labels, no two pairs should be the same." -- i.e., "[foo -> bar, foo -> bar]" is not supported. "[foo -> bar, foo -> baz]" will work (although debatable whether it is a good idea). - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43616/#review119399 --- On Feb. 17, 2016, 6:44 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43616/ > --- > > (Updated Feb. 17, 2016, 6:44 p.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > --- > > The implementation of the equality operator for `Labels` is buggy for labels > that contain duplicates. We might want to revisit fixing the implementation of > that operator (which might be expensive; MESOS-4445), but in the short-term we > should document that duplicates should not be specified. > > > Diffs > - > > include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 > include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 > > Diff: https://reviews.apache.org/r/43616/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Review Request 43712: Documented how the replicated log works.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43712/ --- Review request for mesos, Anand Mazumdar, Jie Yu, and Timothy Chen. Bugs: MESOS-1471 https://issues.apache.org/jira/browse/MESOS-1471 Repository: mesos Description --- This is closely based on an (unpublished) blog post by Jie Yu. Diffs - docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 docs/high-availability-framework-guide.md f21f95f24c0e9f3c4376b64e6a5776aafec39172 docs/home.md 982ad28d392570b40b83e9e85d21583b88ff755e docs/images/log-architecture.png PRE-CREATION docs/images/log-cluster.png PRE-CREATION docs/maintenance.md e6bfe0f655581a6a72de4579bd7e5753625c0e51 docs/operational-guide.md 4680ee3397d081acd6f82499703de4456e7ca4f4 docs/replicated-log-internals.md PRE-CREATION Diff: https://reviews.apache.org/r/43712/diff/ Testing --- Previewed on github gist. Thanks, Neil Conway
Re: Review Request 43716: Endpoint documents with title.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43716/#review119637
---
Seems like this patch doesn't update a lot of the endpoint documentation files
-- `slave/state.md`, `registrar/registry.md`, and others.
docs/endpoints/index.md (line 2)
<https://reviews.apache.org/r/43716/#comment180987>
Can we remove the space before the colon here, for consistency?
i.e.,
```
---
title: XYZ
layout: documentation
---
```
- Neil Conway
On Feb. 18, 2016, 11:56 a.m., Abhishek Dasgupta wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43716/
> ---
>
> (Updated Feb. 18, 2016, 11:56 a.m.)
>
>
> Review request for mesos, Kevin Klues and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Endpoint documents with title.
>
>
> Diffs
> -
>
> docs/endpoints/files/browse.json.md
> 7c7049344980a16978a25431e713fbfe61e1cc5f
> docs/endpoints/files/browse.md 5aa685ad616116168db852ba49e063115f7671f2
> docs/endpoints/files/debug.json.md 3e41fec14d014a46d3a8a0ffddf7f162a39b1347
> docs/endpoints/files/debug.md f3ff3819b14fad3aa9ddf70169c2955d18595e85
> docs/endpoints/files/download.json.md
> 77c6b97988c30ddcd71d69da1ffa55a10e871051
> docs/endpoints/files/download.md 2b8b3f564a55b18bb84d0268b4f7a20e92b05bea
> docs/endpoints/files/read.json.md f86f22727a7d746e8047560a88f15ab82864c062
> docs/endpoints/files/read.md 31dd90cf0c834aca3d130f5e856fb19c7d8500cc
> docs/endpoints/index.md 69d2157f1edf96f608e1d6eaf5a81f2421286415
> docs/endpoints/logging/toggle.md baa4d1b60ed7c55b75f12fdf4e2c10d062bfcb48
> docs/endpoints/master/api/v1/scheduler.md
> 6faa1c2449acc54a5dc0a240959ed70a9cd7c237
> docs/endpoints/master/create-volumes.md
> 1e8fd20dc842defc0a3d22e4f19ddbe3a685cb53
> docs/endpoints/master/destroy-volumes.md
> 7209a7cf788116a29eb6235d3a8a0225253c04f7
> docs/endpoints/master/flags.md b63b6e2fc837aa59341d38dab96c14bd9ed63c46
> docs/endpoints/master/frameworks.md
> bc21f1e3818cf259a5ee2da258afb29afdb7b82a
> docs/endpoints/master/health.md 39af4f963c8d84d64d4c9dafa89f4e9129242f77
> docs/endpoints/master/machine/down.md
> 82cce61e2a02f7896e7db351bed7a08138e87768
> docs/endpoints/master/machine/up.md
> 5bfd95e0945d82030ee536ee247665c455629a64
> docs/endpoints/master/maintenance/schedule.md
> e91ee81a07b09b36db9c3c9eff36f0dbb515fdd0
> docs/endpoints/master/maintenance/status.md
> 17e3eef1c2fac12375892ec125a727a62a4ebfca
> docs/endpoints/master/observe.md acdc18c65798e90459b2b595cc3c72a11f739be2
> docs/endpoints/master/quota.md 26c7bb162f29db1542a4ac2d61368724436e835a
> docs/endpoints/master/redirect.md 4a230e4b7438f8b265c4f5d0a2e5b91f888b39fe
> docs/endpoints/master/reserve.md a71eb8e1800acea0890510ba8d988a7f09047778
> docs/endpoints/master/roles.json.md
> d67779c246cceae2209f2611f32ada4493ae6f83
> docs/endpoints/master/roles.md 976a9b7891a17652289126ec7e7ee73cea0c2e35
> docs/endpoints/master/slaves.md 0be05d79d6480038ca5cfc088b7b476315514027
> docs/endpoints/master/state-summary.md
> a6d79f0e5703c3f9118869e287fbb512e86c22c0
> docs/endpoints/master/state.json.md
> dad5627eea9481fdbfd91966062c813be7e0f586
> docs/endpoints/master/state.md 7fdd5f72eb621fd37e9ec32fc73f1bf50bd5d488
> docs/endpoints/master/tasks.json.md
> cb1856f296d7420ce3162a60bf634de0991cdde4
> docs/endpoints/master/tasks.md e8dbf9370433ee34fc475f5dfebfc15d3b5c62e1
> docs/endpoints/master/teardown.md 9cd86399b532d79d0b1da451320c7f01b948d513
> docs/endpoints/master/unreserve.md 5de7734f86bc61583f06df3a7c02646bf02d01e0
> docs/endpoints/metrics/snapshot.md ab37ab47e4a1692d805698b45d101905029747b5
> docs/endpoints/monitor/statistics.json.md
> 5ce4fc69aaa4b54541841e58ffa29703363b73e2
> docs/endpoints/monitor/statistics.md
> 602104b2484022cfa7f41b04affc106703e6f09f
> docs/endpoints/profiler/start.md 244fd6f6e4695165ff23bc33302b76974bc3f321
> docs/endpoints/profiler/stop.md 6b9738abd8a0b4247fbd1dfd7c3c145cf1b51f9f
> docs/endpoints/registrar/registry.md
> 12b11fe62edfe47cc639fd5cd5224c04d93a24f9
> docs/endpoints/slave/api/v1/executor.md
> e92df49b0a50e0152e54866e812438c9af63c4e0
> docs/endpoints/slave/flags.md 8abbc72f14854cf2cdaab37f9858e9427394ea7e
> docs/endpoints/slave/health.md 265dcfaaa46dfe86dcf8ed7c5357e1ac05bb1dae
> docs/endpoints/slave/state.json.md 0a31159079cf28c
Re: Review Request 38117: Added per container SNMP statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38117/#review119643 --- Seems like this commit should have updated the `v1` version of `mesos.proto`. - Neil Conway On Feb. 11, 2016, 12:48 a.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38117/ > --- > > (Updated Feb. 11, 2016, 12:48 a.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-3365 > https://issues.apache.org/jira/browse/MESOS-3365 > > > Repository: mesos > > > Description > --- > > Added per container SNMP statistics. > > > Diffs > - > > docs/configuration.md b26a058001dd8419b701a3cbea063a9d58b9 > include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > ebf820a2813eef32416f1465eff3f6eea153492f > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > 1c2fbe934baabd1d816018de0c077bc9f63e9d33 > src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d > src/slave/flags.cpp 2ce7f921c9f53f360143b6927d0aaf78a8b958e7 > src/tests/containerizer/port_mapping_tests.cpp > aa9846097feda1a82131b67aa4c782ec3625d049 > > Diff: https://reviews.apache.org/r/38117/diff/ > > > Testing > --- > > ./src/mesos-network-helper statistics --eth0_name=X --enable_snmp_statistics > --pid= > > > Thanks, > > Cong Wang > >
Re: Review Request 43716: Endpoint documents with title.
> On Feb. 18, 2016, 5:30 p.m., Neil Conway wrote: > > Seems like this patch doesn't update a lot of the endpoint documentation > > files -- `slave/state.md`, `registrar/registry.md`, and others. > > Abhishek Dasgupta wrote: > I don't quite understand this comment. As I see, those files are updated > also! Can you mark the lines?? My mistake! I misread the RR. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43716/#review119637 --- On Feb. 18, 2016, 6:38 p.m., Abhishek Dasgupta wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43716/ > --- > > (Updated Feb. 18, 2016, 6:38 p.m.) > > > Review request for mesos, Kevin Klues and Neil Conway. > > > Repository: mesos > > > Description > --- > > Endpoint documents with title. > > > Diffs > - > > docs/endpoints/files/browse.json.md > 7c7049344980a16978a25431e713fbfe61e1cc5f > docs/endpoints/files/browse.md 5aa685ad616116168db852ba49e063115f7671f2 > docs/endpoints/files/debug.json.md 3e41fec14d014a46d3a8a0ffddf7f162a39b1347 > docs/endpoints/files/debug.md f3ff3819b14fad3aa9ddf70169c2955d18595e85 > docs/endpoints/files/download.json.md > 77c6b97988c30ddcd71d69da1ffa55a10e871051 > docs/endpoints/files/download.md 2b8b3f564a55b18bb84d0268b4f7a20e92b05bea > docs/endpoints/files/read.json.md f86f22727a7d746e8047560a88f15ab82864c062 > docs/endpoints/files/read.md 31dd90cf0c834aca3d130f5e856fb19c7d8500cc > docs/endpoints/index.md 69d2157f1edf96f608e1d6eaf5a81f2421286415 > docs/endpoints/logging/toggle.md baa4d1b60ed7c55b75f12fdf4e2c10d062bfcb48 > docs/endpoints/master/api/v1/scheduler.md > 6faa1c2449acc54a5dc0a240959ed70a9cd7c237 > docs/endpoints/master/create-volumes.md > 1e8fd20dc842defc0a3d22e4f19ddbe3a685cb53 > docs/endpoints/master/destroy-volumes.md > 7209a7cf788116a29eb6235d3a8a0225253c04f7 > docs/endpoints/master/flags.md b63b6e2fc837aa59341d38dab96c14bd9ed63c46 > docs/endpoints/master/frameworks.md > bc21f1e3818cf259a5ee2da258afb29afdb7b82a > docs/endpoints/master/health.md 39af4f963c8d84d64d4c9dafa89f4e9129242f77 > docs/endpoints/master/machine/down.md > 82cce61e2a02f7896e7db351bed7a08138e87768 > docs/endpoints/master/machine/up.md > 5bfd95e0945d82030ee536ee247665c455629a64 > docs/endpoints/master/maintenance/schedule.md > e91ee81a07b09b36db9c3c9eff36f0dbb515fdd0 > docs/endpoints/master/maintenance/status.md > 17e3eef1c2fac12375892ec125a727a62a4ebfca > docs/endpoints/master/observe.md acdc18c65798e90459b2b595cc3c72a11f739be2 > docs/endpoints/master/quota.md 26c7bb162f29db1542a4ac2d61368724436e835a > docs/endpoints/master/redirect.md 4a230e4b7438f8b265c4f5d0a2e5b91f888b39fe > docs/endpoints/master/reserve.md a71eb8e1800acea0890510ba8d988a7f09047778 > docs/endpoints/master/roles.json.md > d67779c246cceae2209f2611f32ada4493ae6f83 > docs/endpoints/master/roles.md 976a9b7891a17652289126ec7e7ee73cea0c2e35 > docs/endpoints/master/slaves.md 0be05d79d6480038ca5cfc088b7b476315514027 > docs/endpoints/master/state-summary.md > a6d79f0e5703c3f9118869e287fbb512e86c22c0 > docs/endpoints/master/state.json.md > dad5627eea9481fdbfd91966062c813be7e0f586 > docs/endpoints/master/state.md 7fdd5f72eb621fd37e9ec32fc73f1bf50bd5d488 > docs/endpoints/master/tasks.json.md > cb1856f296d7420ce3162a60bf634de0991cdde4 > docs/endpoints/master/tasks.md e8dbf9370433ee34fc475f5dfebfc15d3b5c62e1 > docs/endpoints/master/teardown.md 9cd86399b532d79d0b1da451320c7f01b948d513 > docs/endpoints/master/unreserve.md 5de7734f86bc61583f06df3a7c02646bf02d01e0 > docs/endpoints/metrics/snapshot.md ab37ab47e4a1692d805698b45d101905029747b5 > docs/endpoints/monitor/statistics.json.md > 5ce4fc69aaa4b54541841e58ffa29703363b73e2 > docs/endpoints/monitor/statistics.md > 602104b2484022cfa7f41b04affc106703e6f09f > docs/endpoints/profiler/start.md 244fd6f6e4695165ff23bc33302b76974bc3f321 > docs/endpoints/profiler/stop.md 6b9738abd8a0b4247fbd1dfd7c3c145cf1b51f9f > docs/endpoints/registrar/registry.md > 12b11fe62edfe47cc639fd5cd5224c04d93a24f9 > docs/endpoints/slave/api/v1/executor.md > e92df49b0a50e0152e54866e812438c9af63c4e0 > docs/endpoints/slave/flags.md 8abbc72f14854cf2cdaab37f9858e9427394ea7e > docs/endpoints/slave/health.md 265dcfaaa46dfe86dcf8ed7c5357e1ac05bb1dae > docs/endpoints/slave/state.json.md 0a31159079cf28cd5b2
Review Request 43737: Fixed typo in fetcher docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43737/ --- Review request for mesos and Bernd Mathiske. Repository: mesos Description --- Fixed typo in fetcher docs. Diffs - docs/fetcher.md 30f9d0f7f622db7ac960c5bf255319c2553b94ee Diff: https://reviews.apache.org/r/43737/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 43384: Fixed minor bug in generate-endpoint-help.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43384/ --- (Updated Feb. 19, 2016, 7:21 p.m.) Review request for mesos, Ben Mahler and Kevin Klues. Changes --- Revised per Kevin's comments. Summary (updated) - Fixed minor bug in generate-endpoint-help.py. Repository: mesos Description (updated) --- If the script exited when `current_subprocess` was None, it would print an ugly exception stack trace. Diffs (updated) - support/generate-endpoint-help.py 28333847e5603c942f25ec9d9a0429bd676f4541 Diff: https://reviews.apache.org/r/43384/diff/ Testing --- Ran the script in error and non-error cases with the changes applied. Thanks, Neil Conway
Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.
> On Feb. 19, 2016, 6:32 a.m., Adam B wrote: > > include/mesos/mesos.proto, line 1600 > > <https://reviews.apache.org/r/43616/diff/2/?file=1252884#file1252884line1600> > > > > Is it ok for labels to contain duplicate keys even if the values are > > different? > > That sounds like undefined behavior too. Is the label-consumer supposed > > to use all values, first value, last value? To me, the issue isn't how the label-consumer is supposed to interpret the labels: rather, labels with duplicate key-value pairs are not handled correctly by Mesos (our equality operator is wrong for this situation -- see MESOS-4445). The initial feeling was that the runtime cost of fixing the equality operator wasn't worth it (although based on the experiments in https://reviews.apache.org/r/43686/, it is unclear whether this is true). The equality operator behaves correctly for labels with duplicate keys but distinct values associated with those keys. How label consumers are supposed to interpret them is up to the application. > On Feb. 19, 2016, 6:32 a.m., Adam B wrote: > > include/mesos/mesos.proto, line 1634 > > <https://reviews.apache.org/r/43616/diff/2/?file=1252884#file1252884line1634> > > > > Are you intentionally leaving out the Labels field in DiscoveryInfo, > > Port, NetworkInfo, Image.Appc? I didn't think Mesos interpreted any of > > these either. It just seemed a bit verbose to copy the same comment that many times, so I settled for documenting the most common use-sites as well as the definition of `Labels`. Happy to change that if people would rather see it done another way though. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43616/#review119822 --- On Feb. 17, 2016, 6:44 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43616/ > --- > > (Updated Feb. 17, 2016, 6:44 p.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > --- > > The implementation of the equality operator for `Labels` is buggy for labels > that contain duplicates. We might want to revisit fixing the implementation of > that operator (which might be expensive; MESOS-4445), but in the short-term we > should document that duplicates should not be specified. > > > Diffs > - > > include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 > include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 > > Diff: https://reviews.apache.org/r/43616/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 42877: Cleaned up MesosSchedulerDriver shutdown in unit tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42877/ --- (Updated Feb. 19, 2016, 7:38 p.m.) Review request for mesos, Greg Mann and Joris Van Remoortere. Changes --- Rebase. Repository: mesos Description --- For consistency, we should do `driver.stop(); driver.join();` if a `MesosSchedulerDriver` has been started by the unit test. The destructor for `MesosSchedulerDriver` will do something _similar_, so the consequence of omitting these calls is not dire, but it seems safer to be consistent between all the test cases. Diffs (updated) - src/tests/executor_http_api_tests.cpp 36a042ed103271ca873450236f39a8152fbbf07e src/tests/oversubscription_tests.cpp d4ae81972fd218c58a413d1968a4e9acbee52fd3 src/tests/reservation_endpoints_tests.cpp afe81b1d38a1b3a82583720f26482ddcde8f5e85 src/tests/scheduler_event_call_tests.cpp bd8920fa9d5475e5f6533c8424ebff1588bfe645 src/tests/scheduler_http_api_tests.cpp 9eb1de7d9541395b92b951f0fe0ddbb2f219fe30 src/tests/slave_tests.cpp c7f5a701eff2c2f9aa3df5722583a131bf2c072a Diff: https://reviews.apache.org/r/42877/diff/ Testing --- make check Note that adding the `driver.stop(); driver.join();` calls to two of the slave tests requires adding an extra shutdown expectation to avoid a GMock warning. Thanks, Neil Conway
Re: Review Request 43730: Added SNMP statistics to v1 mesos.proto too.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43730/#review119957 --- Ship it! Ship It! - Neil Conway On Feb. 18, 2016, 7:15 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43730/ > --- > > (Updated Feb. 18, 2016, 7:15 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Neil Conway. > > > Repository: mesos > > > Description > --- > > As Neil Conway noticed, we should add these SNMP statistics to v1 mesos.proto > too. > > > Diffs > - > > include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 > > Diff: https://reviews.apache.org/r/43730/diff/ > > > Testing > --- > > make > > > Thanks, > > Cong Wang > >
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 19, 2016, 10:27 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Various fixes: introduce functions, replace "long" with "long long", etc. 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 (updated) - docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 include/mesos/mesos.proto 636550f255a122d7f714dbd58f587bea8221b461 include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 src/master/allocator/mesos/hierarchical.cpp 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d 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 (updated) --- 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. PERFORMANCE: This is the performance of the `DeclineOffers` benchmark WITHOUT this RR applied (optimized build on my laptop): ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.192425secs to make 200 offers round 1 allocate took 2.221243secs to make 200 offers round 2 allocate took 2.236314secs to make 200 offers round 3 allocate took 2.224045secs to make 200 offers round 4 allocate took 2.232822secs to make 200 offers round 5 allocate took 2.264807secs to make 200 offers round 6 allocate took 2.224853secs to make 200 offers round 7 allocate took 2.224829secs to make 200 offers round 8 allocate took 2.24862secs to make 200 offers round 9 allocate took 2.2556secs to make 200 offers round 10 allocate took 2.256616secs to make 200 offers ``` And after applying this RR: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.267919secs to make 200 offers round 1 allocate took 2.202843secs to make 200 offers round 2 allocate took 2.20426secs to make 200 offers round 3 allocate took 2.263887secs to make 200 offers round 4 allocate took 2.266237secs to make 200 offers round 5 allocate took 2.276957secs to make 200 offers round 6 allocate took 2.291821secs to make 200 offers round 7 allocate took 2.261839secs to make 200 offers round 8 allocate took 2.325696secs to make 200 offers round 9 allocate took 2.310469secs to make 200 offers round 10 allocate took 2.21802secs to make 200 offers ``` Which suggests to me that any performance hit is pretty minimal. Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
> On Feb. 17, 2016, 12:30 a.m., Klaus Ma wrote: > > src/common/values.cpp, line 59 > > <https://reviews.apache.org/r/43635/diff/1/?file=1252182#file1252182line59> > > > > Replace 1000 with const integer I refactored the code to use functions rather than repeating `1000` in a bunch of places, which I think addresses this concern. > 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. > > Neil Conway wrote: > Truncation rather than rounding might also be reasonable behavior; I'm > curious what other people think. Chatting with Joris, our feeling was that it is hard to do something that is right for everyone, but that the risk of overcommitting resources due to rounding is probably minimal. Overall rounding probably seems like a better policy than either taking the floor or the ceiling. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review119382 --- On Feb. 19, 2016, 10:27 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43635/ > --- > > (Updated Feb. 19, 2016, 10:27 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 636550f255a122d7f714dbd58f587bea8221b461 > include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 > src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc > src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 > src/master/allocator/mesos/hierarchical.cpp > 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d > 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. > > PERFORMANCE
Re: Review Request 43792: Made bullet point structure consistent in upgrades.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43792/#review119995 --- Ship it! Ship It! - Neil Conway On Feb. 20, 2016, 12:03 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43792/ > --- > > (Updated Feb. 20, 2016, 12:03 a.m.) > > > Review request for mesos, Michael Park and Neil Conway. > > > Repository: mesos > > > Description > --- > > Made bullet point structure consistent in upgrades.md. > > > Diffs > - > > docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 > > Diff: https://reviews.apache.org/r/43792/diff/ > > > Testing > --- > > Viewed in github (https://gist.github.com/joerg84/3cd9077f3446a6c6bb50) and > via docker website renderer. > > > Thanks, > > Joerg Schad > >
Re: Review Request 43796: Added documentation for `cgroups/net_cls` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43796/#review11 --- docs/mesos-containerizer.md (line 91) <https://reviews.apache.org/r/43796/#comment181361> I'd remove the commas here. docs/mesos-containerizer.md (line 92) <https://reviews.apache.org/r/43796/#comment181362> "Mesos" docs/mesos-containerizer.md (line 96) <https://reviews.apache.org/r/43796/#comment181363> "Linux" No comma before "and" docs/mesos-containerizer.md (line 102) <https://reviews.apache.org/r/43796/#comment181365> "specified" docs/mesos-containerizer.md (line 114) <https://reviews.apache.org/r/43796/#comment181366> We should try to avoid link anchor text like "here"; it would be better to write a normal sentence and then link the appropriate part. e.g., "The [net_cls documentation](XXX) has more information on YYY..." docs/mesos-containerizer.md (line 117) <https://reviews.apache.org/r/43796/#comment181367> Remove comma docs/mesos-containerizer.md (line 118) <https://reviews.apache.org/r/43796/#comment181369> "net_cls handles"? docs/mesos-containerizer.md (line 128) <https://reviews.apache.org/r/43796/#comment181368> Remove comma - Neil Conway On Feb. 20, 2016, 12:28 a.m., Avinash sridharan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43796/ > --- > > (Updated Feb. 20, 2016, 12:28 a.m.) > > > Review request for mesos, Jie Yu and Neil Conway. > > > Bugs: MESOS-4660 > https://issues.apache.org/jira/browse/MESOS-4660 > > > Repository: mesos > > > Description > --- > > Added documentation for `cgroups/net_cls` isolator. > > > Diffs > - > > docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 > > Diff: https://reviews.apache.org/r/43796/diff/ > > > Testing > --- > > Built the web-site using docker, and proof read the website and links on > localhost. > > Verified all the links embedded in markdown work. > > > Thanks, > > Avinash sridharan > >
Review Request 43816: Updated `/frameworks` endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43816/ --- Review request for mesos and Michael Park. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- Updated `/frameworks` endpoint to use jsonify. Diffs - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43816/diff/ Testing --- 1. make check 2. Verified that after introducing a bug into the jsonify version of `frameworks()`, `make check` fails (i.e., the test suite covers the `/frameworks` endpoint). 3. Compared output of `/frameworks` with old and new implementation on a test Mesos installation to try to gauge correctness visually. Thanks, Neil Conway
reviews@mesos.apache.org
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43817/ --- Review request for mesos and Michael Park. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- These provided functionality that is now implemented via jsonify; no more call-sites of the old functions remain. Diffs - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43817/diff/ Testing --- make check Thanks, Neil Conway
reviews@mesos.apache.org
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43817/ --- (Updated Feb. 22, 2016, 2:11 a.m.) Review request for mesos and Michael Park. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- These provided functionality that is now implemented via jsonify; no more call-sites of the old functions remain. Diffs - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43817/diff/ Testing (updated) --- make check Also updated a few comments to not refer to `summarize`. Thanks, Neil Conway
Re: Review Request 43821: Updated the HA framweork guide for TASK_KILLING.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43821/#review120110 --- Ship it! docs/high-availability-framework-guide.md (line 204) <https://reviews.apache.org/r/43821/#comment181486> "has not yet been killed" ? docs/high-availability-framework-guide.md (line 206) <https://reviews.apache.org/r/43821/#comment181485> "`FrameworkInfo.capabilities`" ? Typo in commit summary. - Neil Conway On Feb. 22, 2016, 5:19 a.m., Ben Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43821/ > --- > > (Updated Feb. 22, 2016, 5:19 a.m.) > > > Review request for mesos, Abhishek Dasgupta and Neil Conway. > > > Bugs: MESOS-4547 > https://issues.apache.org/jira/browse/MESOS-4547 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > docs/high-availability-framework-guide.md > 0d9c483985d61b512339f50f395f9360de034e2d > > Diff: https://reviews.apache.org/r/43821/diff/ > > > Testing > --- > > N/A > > > Thanks, > > Ben Mahler > >
Re: Review Request 43816: Updated `/frameworks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43816/ --- (Updated Feb. 22, 2016, 7:11 a.m.) Review request for mesos and Michael Park. Summary (updated) - Updated `/frameworks` master endpoint to use jsonify. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description (updated) --- Updated `/frameworks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43816/diff/ Testing --- 1. make check 2. Verified that after introducing a bug into the jsonify version of `frameworks()`, `make check` fails (i.e., the test suite covers the `/frameworks` endpoint). 3. Compared output of `/frameworks` with old and new implementation on a test Mesos installation to try to gauge correctness visually. Thanks, Neil Conway
Review Request 43823: Updated `/tasks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43823/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Updated `/tasks` master endpoint to use jsonify. Diffs - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43823/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Review Request 43822: Updated `/slaves` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43822/ --- Review request for mesos. Repository: mesos Description --- Updated `/slaves` master endpoint to use jsonify. Diffs - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43822/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Re: Review Request 43817: Removed no-longer-used model functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43817/ --- (Updated Feb. 22, 2016, 7:13 a.m.) Review request for mesos and Michael Park. Summary (updated) - Removed no-longer-used model functions. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description (updated) --- These `model()` variants provided functionality that is now implemented via jsonify; no more call-sites of the old functions remain. Diffs (updated) - src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 Diff: https://reviews.apache.org/r/43817/diff/ Testing --- make check Also updated a few comments to not refer to `summarize`. Thanks, Neil Conway
Re: Review Request 43826: Added 'Synchronized Statement in Mesos' blog post.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43826/#review120166
---
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 15)
<https://reviews.apache.org/r/43826/#comment181565>
"Java language feature" -- or in any case I'd probably drop the hyphen.
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 35)
<https://reviews.apache.org/r/43826/#comment181566>
"class and"
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 42)
<https://reviews.apache.org/r/43826/#comment181579>
I'm a little confused by the semantics of the proposed `if` statement --
you're saying the enclosing block only executes if `condition` is true?
Syntactically it seems weird as well.
How about
```
{
if_guard my_if(condition);
// Block only executes if `condition` is true
}
```
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 54)
<https://reviews.apache.org/r/43826/#comment181593>
No comma
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 57)
<https://reviews.apache.org/r/43826/#comment181580>
Remove the comma.
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 64)
<https://reviews.apache.org/r/43826/#comment181583>
Seems a little unclear what you're referring to by "the patterns involving
other RAII classes such as `std::vector`"
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 115)
<https://reviews.apache.org/r/43826/#comment181601>
"scope and then destroyed"
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 121)
<https://reviews.apache.org/r/43826/#comment181602>
Remove comma.
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 276)
<https://reviews.apache.org/r/43826/#comment181610>
"If"
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 286)
<https://reviews.apache.org/r/43826/#comment181611>
"Remembering all of these rules is simply too complicated, so we opt to use
a more clever solution."
site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 292)
<https://reviews.apache.org/r/43826/#comment181613>
The usage "a control flow" is a bit unusual (at least to me). What about "a
program fragment that is easier for the compiler to reason about"?
- Neil Conway
On Feb. 22, 2016, 7:50 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43826/
> ---
>
> (Updated Feb. 22, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Joerg Schad and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> site/source/blog/2016-02-16-synchronized-statements-in-mesos.md
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43826/diff/
>
>
> Testing
> ---
>
> Locally rendered with `rake && rake dev` from `/site`
>
>
> Thanks,
>
> Michael Park
>
>
Review Request 43848: Used `size_t` to track number of frameworks per role.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43848/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Per informal project style, we prefer using unsigned types to represent non-negative quantities. Diffs - src/master/allocator/mesos/hierarchical.hpp 0d39d3f3b5f4ff7f62f9de7200d062845c71818a Diff: https://reviews.apache.org/r/43848/diff/ Testing --- make check on OSX Compilation on Linux with GCC 5.3 Thanks, Neil Conway
Re: Review Request 43826: Added 'Synchronized Statement in Mesos' blog post.
> On Feb. 22, 2016, 7:30 p.m., Neil Conway wrote: > > site/source/blog/2016-02-16-synchronized-statements-in-mesos.md, line 286 > > <https://reviews.apache.org/r/43826/diff/1/?file=1264057#file1264057line286> > > > > "Remembering all of these rules is simply too complicated, so we opt to > > use a more clever solution." > > Benjamin Bannier wrote: > Stongly disagree: *clever* carries a negative connotation, while *solve > properly* is positive. "So we opt to solve the problem in a different manner." is fine with me also -- I was more suggesting the sentence flow be rephrased. > On Feb. 22, 2016, 7:30 p.m., Neil Conway wrote: > > site/source/blog/2016-02-16-synchronized-statements-in-mesos.md, line 292 > > <https://reviews.apache.org/r/43826/diff/1/?file=1264057#file1264057line292> > > > > The usage "a control flow" is a bit unusual (at least to me). What > > about "a program fragment that is easier for the compiler to reason about"? > > Benjamin Bannier wrote: > IamNotAComputerScientist, but I believe this is standard compiler lingo > (cf. *control flow graph*). "control flow" is certainly standard, I just haven't seen it usually referred to as "a control flow" (in CFG, the object is a "graph"; a CFG is a type of graph). - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43826/#review120166 --- On Feb. 22, 2016, 7:48 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43826/ > --- > > (Updated Feb. 22, 2016, 7:48 p.m.) > > > Review request for mesos, Benjamin Bannier, Joerg Schad, and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > site/source/blog/2016-02-16-synchronized-statements-in-mesos.md > PRE-CREATION > > Diff: https://reviews.apache.org/r/43826/diff/ > > > Testing > --- > > Locally rendered with `rake && rake dev` from `/site` > > > Thanks, > > Michael Park > >
Re: Review Request 43639: Allowed dynamic reservation without a principal.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43639/#review120209
---
Fix it, then Ship it!
src/tests/reservation_tests.cpp (line 1851)
<https://reviews.apache.org/r/43639/#comment181635>
This should probably be split into two test cases, since AFAICS the first
and second halves of the test don't share any state.
src/tests/reservation_tests.cpp (line 1949)
<https://reviews.apache.org/r/43639/#comment181634>
Not yours, but I would probably opt for `using Process::Clock;` at the top
of the file.
- Neil Conway
On Feb. 22, 2016, 6:39 p.m., Greg Mann wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43639/
> ---
>
> (Updated Feb. 22, 2016, 6:39 p.m.)
>
>
> Review request for mesos, Michael Park and Neil Conway.
>
>
> Bugs: MESOS-3940
> https://issues.apache.org/jira/browse/MESOS-3940
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Allowed dynamic reservation without a principal.
>
> The `ReservationInfo.principal` field has been migrated to `optional`, which
> means we can now allow dynamic reservation and unreservation without a
> principal. This allows the use of the `/reserve` and `/unreserve` HTTP
> endpoints when HTTP authentication is disabled.
>
> Note that we still require that frameworks/operators set the
> `ReservationInfo.principal` field to match their own principal, if present.
> It may be desirable to remove this requirement; this improvement is tracked
> in MESOS-4696.
>
>
> Diffs
> -
>
> src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca
> src/tests/master_validation_tests.cpp
> 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5
> src/tests/reservation_endpoints_tests.cpp
> afe81b1d38a1b3a82583720f26482ddcde8f5e85
> src/tests/reservation_tests.cpp d2ef15934556cb879f31850d52712aec77231fc7
>
> Diff: https://reviews.apache.org/r/43639/diff/
>
>
> Testing
> ---
>
> A new test case was added to `ReservationTest.NoAuthentication`.
>
> `make check` was used to test on OSX, both with and without SSL enabled.
>
> Also manually reserved/unreserved resources using curl, with a command like
> this: `curl -i -d slaveId="8288b2f0-e33d-4547-a2b4-5230ba6e5279-S0" -d
> resources='[ { "name": "cpus", "type": "SCALAR", "scalar": { "value": 3 },
> "role": "ads", "reservation": { } } ]' -X POST
> http://127.0.0.1:5050/master/reserve`
>
> Inspecting `/master/state` before & after these operations confirmed that the
> reserve/unreserve operations were successful.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 43641: Removed unnecessary parameter from validation function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43641/#review120210 --- Ship it! Ship It! - Neil Conway On Feb. 22, 2016, 6:43 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43641/ > --- > > (Updated Feb. 22, 2016, 6:43 p.m.) > > > Review request for mesos, Michael Park and Neil Conway. > > > Bugs: MESOS-3940 > https://issues.apache.org/jira/browse/MESOS-3940 > > > Repository: mesos > > > Description > --- > > Removed unnecessary parameter from validation function. > > Since unreserve operations are now possible without a principal, the `bool > hasPrincipal` parameter to the Unreserve operation validation function is no > longer necessary. > > > Diffs > - > > src/master/http.cpp ae6bc7852202480e58f579a5b48ab5b5e5ff9317 > src/master/master.cpp b453bc7fca05c192df616b7d80132985b3248547 > src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c > src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca > src/tests/master_validation_tests.cpp > 6fae01fa1833ae05ec82618a4ae28ac5bd275bd5 > > Diff: https://reviews.apache.org/r/43641/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 43642: Updated comments and docs for '/(un)reserve' without principal.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43642/#review120211 --- Ship it! Ship It! - Neil Conway On Feb. 22, 2016, 6:44 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43642/ > --- > > (Updated Feb. 22, 2016, 6:44 p.m.) > > > Review request for mesos, Michael Park and Neil Conway. > > > Bugs: MESOS-3940 > https://issues.apache.org/jira/browse/MESOS-3940 > > > Repository: mesos > > > Description > --- > > Updated comments and docs for '/reserve' and '/unreserve' without principal. > > > Diffs > - > > docs/reservation.md 41321d436d3a90475bcce551dd9af2adeb2e68d6 > include/mesos/mesos.proto 11a71cbe25acbc232cea6b5d72484e2e9eef6167 > include/mesos/v1/mesos.proto 84e933e0bc30aa8f9b6d6047402f449666a80a23 > > Diff: https://reviews.apache.org/r/43642/diff/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 43864: Fix typo of roles doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43864/#review120347 --- The right place to make this fix is in `src/master/http.cpp`, and then rerun `support/generate-endpoint-help.py`. Seems like `Master::Http::ROLES_HELP` is missing commas in `DESCRIPTION`. - Neil Conway On Feb. 23, 2016, 3:20 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43864/ > --- > > (Updated Feb. 23, 2016, 3:20 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Fix typo of roles doc. > > > Diffs > - > > docs/endpoints/master/roles.json.md > d67779c246cceae2209f2611f32ada4493ae6f83 > docs/endpoints/master/roles.md 976a9b7891a17652289126ec7e7ee73cea0c2e35 > > Diff: https://reviews.apache.org/r/43864/diff/ > > > Testing > --- > > > Thanks, > > Klaus Ma > >
Re: Review Request 43384: Fixed minor bug in generate-endpoint-help.py.
> On Feb. 23, 2016, 6:17 p.m., Ben Mahler wrote: > > support/generate-endpoint-help.py, line 316 > > <https://reviews.apache.org/r/43384/diff/2/?file=1255050#file1255050line316> > > > > How about on_exit? Sounds good. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43384/#review120350 --- On Feb. 19, 2016, 7:21 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43384/ > --- > > (Updated Feb. 19, 2016, 7:21 p.m.) > > > Review request for mesos, Ben Mahler and Kevin Klues. > > > Repository: mesos > > > Description > --- > > If the script exited when `current_subprocess` was None, it would > print an ugly exception stack trace. > > > Diffs > - > > support/generate-endpoint-help.py 28333847e5603c942f25ec9d9a0429bd676f4541 > > Diff: https://reviews.apache.org/r/43384/diff/ > > > Testing > --- > > Ran the script in error and non-error cases with the changes applied. > > > Thanks, > > Neil Conway > >
Re: Review Request 43819: Added Scheduler-Driver API to app-framework-development-guide.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43819/#review120356 --- Fix it, then Ship it! docs/app-framework-development-guide.md (line 17) <https://reviews.apache.org/r/43819/#comment181787> I'd say "framework schedulers" for simplicity. - Neil Conway On Feb. 22, 2016, 3 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43819/ > --- > > (Updated Feb. 22, 2016, 3 a.m.) > > > Review request for mesos, Adam B and Neil Conway. > > > Repository: mesos > > > Description > --- > > Added Scheduler-Driver API to app-framework-development-guide.md. > > > Diffs > - > > docs/app-framework-development-guide.md > e0f40adacf96bdf0c510b3400eb0ed0cd964ab9d > > Diff: https://reviews.apache.org/r/43819/diff/ > > > Testing > --- > > Viewed via gist (https://gist.github.com/joerg84/b4bf279a55e1b62051e6) and > via docker website container. > > > Thanks, > > Joerg Schad > >
Re: Review Request 43798: Added overview section to upgrades.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43798/#review120374 --- docs/upgrades.md (line 13) <https://reviews.apache.org/r/43798/#comment181813> Seems like this isn't a table any more? Either a table or a bulleted list would be good. - Neil Conway On Feb. 23, 2016, 7:31 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43798/ > --- > > (Updated Feb. 23, 2016, 7:31 p.m.) > > > Review request for mesos, Michael Park, Neil Conway, and Till Toenshoff. > > > Bugs: MESOS-4381 > https://issues.apache.org/jira/browse/MESOS-4381 > > > Repository: mesos > > > Description > --- > > Added overview section to upgrades.md. > > > Diffs > - > > docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 > > Diff: https://reviews.apache.org/r/43798/diff/ > > > Testing > --- > > Viewed via gist (https://gist.github.com/joerg84/eddbc0302a5a4b291e81) and > docker website container. > > > Thanks, > > Joerg Schad > >
Review Request 43911: Updated `/state` agent endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43911/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Updated `/state` agent endpoint to use jsonify. Diffs - src/slave/http.cpp a18085ea020d0d6c39f23213e11af75a02eedb7e Diff: https://reviews.apache.org/r/43911/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43816: Updated `/frameworks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43816/ --- (Updated Feb. 23, 2016, 10:29 p.m.) Review request for mesos and Michael Park. Changes --- Rebase. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- Updated `/frameworks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b Diff: https://reviews.apache.org/r/43816/diff/ Testing --- 1. make check 2. Verified that after introducing a bug into the jsonify version of `frameworks()`, `make check` fails (i.e., the test suite covers the `/frameworks` endpoint). 3. Compared output of `/frameworks` with old and new implementation on a test Mesos installation to try to gauge correctness visually. Thanks, Neil Conway
Review Request 43910: Enhanced a test case for the `/state` agent endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43910/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Enhanced a test case for the `/state` agent endpoint. Diffs - src/tests/slave_tests.cpp c7f5a701eff2c2f9aa3df5722583a131bf2c072a Diff: https://reviews.apache.org/r/43910/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43822: Updated `/slaves` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43822/ --- (Updated Feb. 23, 2016, 10:29 p.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/slaves` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b Diff: https://reviews.apache.org/r/43822/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Re: Review Request 43817: Removed no-longer-used model functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43817/ --- (Updated Feb. 23, 2016, 10:29 p.m.) Review request for mesos and Michael Park. Changes --- Rebase. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- These `model()` variants provided functionality that is now implemented via jsonify; no more call-sites of the old functions remain. Diffs (updated) - src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b Diff: https://reviews.apache.org/r/43817/diff/ Testing --- make check Also updated a few comments to not refer to `summarize`. Thanks, Neil Conway
Re: Review Request 43823: Updated `/tasks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43823/ --- (Updated Feb. 23, 2016, 10:29 p.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/tasks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b Diff: https://reviews.apache.org/r/43823/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Re: Review Request 43864: Fix typo of roles doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43864/#review120531 --- Ship it! Ship It! - Neil Conway On Feb. 24, 2016, 3:34 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43864/ > --- > > (Updated Feb. 24, 2016, 3:34 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Fix typo of roles doc. > > > Diffs > - > > docs/endpoints/master/roles.json.md > d67779c246cceae2209f2611f32ada4493ae6f83 > docs/endpoints/master/roles.md 976a9b7891a17652289126ec7e7ee73cea0c2e35 > src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b > > Diff: https://reviews.apache.org/r/43864/diff/ > > > Testing > --- > > > Thanks, > > Klaus Ma > >
Re: Review Request 43035: Added a test for the interaction between timers and destroyed Groups.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43035/ --- (Updated Feb. 24, 2016, 11:41 p.m.) Review request for mesos and Joris Van Remoortere. Changes --- Rebase. Repository: mesos Description --- Check that even though we might fire a timer for a `GroupProcess` that has been destroyed, this does not result in dispatching an event to a reclaimed process. Diffs (updated) - src/tests/group_tests.cpp af530f32fac47801a2cd0d941f3aa9196d448bd2 Diff: https://reviews.apache.org/r/43035/diff/ Testing --- `make check; ./src/mesos-tests --gtest_filter="GroupTest.TimerCleanup" --gtest_repeat=4000 --gtest_break_on_failure` I also verified that if you remove the `delete` of the `GroupProcess`, the test fails because the `expired` callback is invoked (the `Clock::settle()` is necessary to ensure that this happens). Thanks, Neil Conway
Re: Review Request 43974: Fixed compilation error in GroupTest.ConnectTimer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43974/#review120661 --- Ship it! Ship It! - Neil Conway On Feb. 25, 2016, 2:04 a.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43974/ > --- > > (Updated Feb. 25, 2016, 2:04 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Neil > Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/group_tests.cpp bb548f4681a70717305ccb0ebc4ead643d553e2e > > Diff: https://reviews.apache.org/r/43974/diff/ > > > Testing > --- > > make check (without --disable-java) on Centos 7 & Debian 8 & Ubuntu 14 > > > Thanks, > > Joseph Wu > >
Review Request 44047: Added full reserved resource info to `/slaves` master endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44047/ --- Review request for mesos, Michael Park and Vinod Kone. Bugs: MESOS-4667 https://issues.apache.org/jira/browse/MESOS-4667 Repository: mesos Description --- This allows operators to list all the dynamic reservations and persistent volumes in a cluster. This is important in itself; it also makes it easier to use the `/unreserve` and `/destroy-volumes` endpoints. Diffs - docs/persistent-volume.md 2a794a572ff930aa1f95706b89fef9243be627de docs/reservation.md b98ebe6df0739b48c5fa58e087fd64b1c6c5d456 src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 src/tests/persistent_volume_endpoints_tests.cpp 6069ca1e9ed278459c5182e438417e95955b1924 Diff: https://reviews.apache.org/r/44047/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43826: Added 'Synchronized Statement in Mesos' blog post.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43826/#review120822 --- site/source/blog/2016-02-16-synchronized-statements-in-mesos.md (line 3) <https://reviews.apache.org/r/43826/#comment182327> in Mesos, or in libprocess? - Neil Conway On Feb. 22, 2016, 7:48 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43826/ > --- > > (Updated Feb. 22, 2016, 7:48 p.m.) > > > Review request for mesos, Benjamin Bannier, Joerg Schad, and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > site/source/blog/2016-02-16-synchronized-statements-in-mesos.md > PRE-CREATION > > Diff: https://reviews.apache.org/r/43826/diff/ > > > Testing > --- > > Locally rendered with `rake && rake dev` from `/site` > > > Thanks, > > Michael Park > >
Re: Review Request 43800: Updated docs for reservation, volumes, and authZ.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43800/#review120823 --- This needs a prominent note in `upgrades.md` about the change to the ACL format. Thinking about it, the ACL isn't stored anywhere, so there's no issue with incompatibility of stored state. Similarly, rolling upgrades should be okay -- a mixed cluster would behave in a strange way in the event of master failover, but that's probably to be expected. docs/authorization.md (line 37) <https://reviews.apache.org/r/43800/#comment182328> Not yours, but I feel like we need a better way to organize this information. Maybe a table/matrix, showing the action X is used with subjects Y and objects Z? docs/persistent-volume.md (line 39) <https://reviews.apache.org/r/43800/#comment182329> I'd say "appropriate" rather than "desired". docs/reservation.md (line 54) <https://reviews.apache.org/r/43800/#comment182330> I'd say "appropriate" rather than "desired". - Neil Conway On Feb. 25, 2016, 5:45 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43800/ > --- > > (Updated Feb. 25, 2016, 5:45 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Updated docs for reservation, volumes, and authZ. > > This updates the authorization documentation to include the new `roles` > object for the `CreateVolume` and `ReserveResources` ACLs. The docs for > persistent volumes and dynamic reservations are also updated to reflect the > new authorization behavior. > > > Diffs > - > > docs/authorization.md bbb4f2adc9348cb1686e6af78f5604d8cf7651ab > docs/persistent-volume.md 2a794a572ff930aa1f95706b89fef9243be627de > docs/reservation.md b98ebe6df0739b48c5fa58e087fd64b1c6c5d456 > > Diff: https://reviews.apache.org/r/43800/diff/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 43779: Added '/reserve' tests with multiple roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43779/#review120829 --- src/tests/reservation_endpoints_tests.cpp (line 1069) <https://reviews.apache.org/r/43779/#comment182352> I'd inline this into the lone use-site. src/tests/reservation_endpoints_tests.cpp (line 1228) <https://reviews.apache.org/r/43779/#comment182338> Mark this and the following string `const`. src/tests/reservation_endpoints_tests.cpp (line 1270) <https://reviews.apache.org/r/43779/#comment182347> I'd omit the variable and just call `createBasicAuthHeaders` in the args to `post()`. src/tests/reservation_endpoints_tests.cpp (line 1273) <https://reviews.apache.org/r/43779/#comment182351> The analogous test for volumes names these variables differently (`volume1`, `volume2`, and `volumes`). We should be consistent - Neil Conway On Feb. 26, 2016, 5:31 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43779/ > --- > > (Updated Feb. 26, 2016, 5:31 a.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Added '/reserve' tests with multiple roles. > > Operators may reserve resources for multiple roles in the same operation; > this patch adds tests to confirm correct behavior of authorization in this > case. The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and > `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added. > > > Diffs > - > > src/tests/reservation_endpoints_tests.cpp > 32b2af4115211b58a5127a14dd19152c2eca120c > > Diff: https://reviews.apache.org/r/43779/diff/ > > > Testing > --- > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43778: Added '/create-volumes' tests with multiple roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43778/#review120832 --- src/tests/persistent_volume_endpoints_tests.cpp (line 839) <https://reviews.apache.org/r/43778/#comment182344> `const` src/tests/persistent_volume_endpoints_tests.cpp (line 874) <https://reviews.apache.org/r/43778/#comment182348> Unused variable. src/tests/persistent_volume_endpoints_tests.cpp (line 1066) <https://reviews.apache.org/r/43778/#comment182349> Unused variable. src/tests/persistent_volume_endpoints_tests.cpp (line 1082) <https://reviews.apache.org/r/43778/#comment182350> `response` seems a bit more concise. - Neil Conway On Feb. 24, 2016, 6:42 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43778/ > --- > > (Updated Feb. 24, 2016, 6:42 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Added '/create-volumes' tests with multiple roles. > > Operators may create volumes for multiple roles in the same operation; this > patch adds tests to confirm correct behavior of authorization in this case. > The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and > `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added. > > > Diffs > - > > src/tests/persistent_volume_endpoints_tests.cpp > 6069ca1e9ed278459c5182e438417e95955b1924 > > Diff: https://reviews.apache.org/r/43778/diff/ > > > Testing > --- > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43776: Changed object of `ReserveResources` ACL to `roles`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43776/#review120826 --- src/master/master.cpp (line 2857) <https://reviews.apache.org/r/43776/#comment182353> I might add a note here to remind the reader that authorization only succeeds if the principal is allowed to make reservations for the roles included in the resources. src/master/master.cpp (line 2858) <https://reviews.apache.org/r/43776/#comment182335> "an element" src/master/master.cpp (line 2859) <https://reviews.apache.org/r/43776/#comment182332> Is there a reason to prefer `std::set` over `hashset`? I would typically use `hashset` unless we care about ordered iteration. src/tests/master_validation_tests.cpp (line 236) <https://reviews.apache.org/r/43776/#comment182341> I'd rephrase this comment slightly: "even if the `role`". i.e., as written, it seems to imply that resource-role != framework-role is sufficient for the request to validate successfully. - Neil Conway On Feb. 24, 2016, 6:39 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43776/ > --- > > (Updated Feb. 24, 2016, 6:39 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Changed object of the `ReserveResources` ACL to `roles`. > > This solves a problem in which any principal could reserve resources for any > role using the '/reserve' operator endpoint. A new test, > `ReserveOperationValidationTest.DisallowReserveForStarRole`, was added. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 > src/authorizer/local/authorizer.cpp > 9557bbdf68ff182c4538bbf70cee576d717abc05 > src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 > src/master/validation.cpp b0cc7f7ec75b66246686d1b50a61902f1455e8b6 > src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc > src/tests/master_validation_tests.cpp > ab2df22f73052f6bd77653e56e7b460b17e7b0be > src/tests/reservation_endpoints_tests.cpp > 32b2af4115211b58a5127a14dd19152c2eca120c > src/tests/reservation_tests.cpp b8878d51767ac0d95e346c44c0a4d5c060e565ef > > Diff: https://reviews.apache.org/r/43776/diff/ > > > Testing > --- > > Tests were altered to accomodate the new ACL object, and the test > `ReserveOperationValidationTest.DisallowReserveForStarRole` was added. > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43777: Removed unnecessary parameter from validation function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43777/#review120834 --- Ship it! Ship It! - Neil Conway On Feb. 24, 2016, 6:41 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43777/ > --- > > (Updated Feb. 24, 2016, 6:41 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Removed unnecessary parameter from validation function. > > Now that Reserve operations are authorized for particular roles, it is > unnecessary to pass the framework's role into the validation function for > Reserve operations. The function previously ensured that a framework could > only reserve resources for its own role, but this check has been removed. > Since this check has been removed, the test > `ReserveOperationValidationTest.NonMatchingRole` is no longer needed and has > also been removed. > > > Diffs > - > > src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b > src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 > src/master/validation.hpp 6ec883e7e84162b648c2f04583cc776948b36c0c > src/master/validation.cpp b0cc7f7ec75b66246686d1b50a61902f1455e8b6 > src/tests/master_validation_tests.cpp > ab2df22f73052f6bd77653e56e7b460b17e7b0be > > Diff: https://reviews.apache.org/r/43777/diff/ > > > Testing > --- > > This patch removes the test `ReserveOperationValidationTest.NonMatchingRole`, > as the condition it was testing for is no longer relevant. > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43776: Changed object of `ReserveResources` ACL to `roles`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43776/#review120916 --- Ship it! Ship It! - Neil Conway On Feb. 26, 2016, 6:52 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43776/ > --- > > (Updated Feb. 26, 2016, 6:52 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Changed object of the `ReserveResources` ACL to `roles`. > > This solves a problem in which any principal could reserve resources for any > role using the '/reserve' operator endpoint. A new test, > `ReserveOperationValidationTest.DisallowReserveForStarRole`, was added. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 > src/authorizer/local/authorizer.cpp > 9557bbdf68ff182c4538bbf70cee576d717abc05 > src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 > src/master/validation.cpp b0cc7f7ec75b66246686d1b50a61902f1455e8b6 > src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc > src/tests/master_validation_tests.cpp > ab2df22f73052f6bd77653e56e7b460b17e7b0be > src/tests/reservation_endpoints_tests.cpp > 32b2af4115211b58a5127a14dd19152c2eca120c > src/tests/reservation_tests.cpp b8878d51767ac0d95e346c44c0a4d5c060e565ef > > Diff: https://reviews.apache.org/r/43776/diff/ > > > Testing > --- > > Tests were altered to accomodate the new ACL object, and the test > `ReserveOperationValidationTest.DisallowReserveForStarRole` was added. > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43800: Updated docs for reservation, volumes, and authZ.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43800/#review120918 --- Fix it, then Ship it! docs/upgrades.md (line 11) <https://reviews.apache.org/r/43800/#comment182446> I think we should add an explicit note that previously, a framework could only make reservations for its principal, and that the old behavior can be preserved by configuring ACLs appropriately. - Neil Conway On Feb. 26, 2016, 5:33 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43800/ > --- > > (Updated Feb. 26, 2016, 5:33 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Updated docs for reservation, volumes, and authZ. > > This updates the authorization documentation to include the new `roles` > object for the `CreateVolume` and `ReserveResources` ACLs. The docs for > persistent volumes and dynamic reservations are also updated to reflect the > new authorization behavior. A note has been added to `upgrades.md` detailing > the impact of these changes on upgrades. > > > Diffs > - > > docs/authorization.md bbb4f2adc9348cb1686e6af78f5604d8cf7651ab > docs/persistent-volume.md 2a794a572ff930aa1f95706b89fef9243be627de > docs/reservation.md b98ebe6df0739b48c5fa58e087fd64b1c6c5d456 > docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 > > Diff: https://reviews.apache.org/r/43800/diff/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 43779: Added '/reserve' tests with multiple roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43779/#review120919 --- Ship it! Ship It! - Neil Conway On Feb. 26, 2016, 4:58 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43779/ > --- > > (Updated Feb. 26, 2016, 4:58 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Added '/reserve' tests with multiple roles. > > Operators may reserve resources for multiple roles in the same operation; > this patch adds tests to confirm correct behavior of authorization in this > case. The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and > `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added. > > > Diffs > - > > src/tests/reservation_endpoints_tests.cpp > 32b2af4115211b58a5127a14dd19152c2eca120c > > Diff: https://reviews.apache.org/r/43779/diff/ > > > Testing > --- > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43778: Added '/create-volumes' tests with multiple roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43778/#review120920 --- Ship it! Ship It! - Neil Conway On Feb. 26, 2016, 4:56 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43778/ > --- > > (Updated Feb. 26, 2016, 4:56 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Added '/create-volumes' tests with multiple roles. > > Operators may create volumes for multiple roles in the same operation; this > patch adds tests to confirm correct behavior of authorization in this case. > The tests `ReservationEndpointsTest.GoodReserveACLMultipleRoles` and > `ReservationEndpointsTest.BadReserveACLMultipleRoles` were added. > > > Diffs > - > > src/tests/persistent_volume_endpoints_tests.cpp > 6069ca1e9ed278459c5182e438417e95955b1924 > > Diff: https://reviews.apache.org/r/43778/diff/ > > > Testing > --- > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43782: Changed object of `CreateVolume` ACL to `roles`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43782/#review120923 --- Fix it, then Ship it! src/master/master.cpp (line 2930) <https://reviews.apache.org/r/43782/#comment182448> Should talk about persistent volumes, not reservations. - Neil Conway On Feb. 26, 2016, 6:53 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43782/ > --- > > (Updated Feb. 26, 2016, 6:53 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4591 > https://issues.apache.org/jira/browse/MESOS-4591 > > > Repository: mesos > > > Description > --- > > Changed object of `CreateVolume` ACL to `roles`. > > This solves a problem in which any principal could create volumes for any > role using the '/create-volumes' operator endpoint. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > 226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 > src/authorizer/local/authorizer.cpp > 9557bbdf68ff182c4538bbf70cee576d717abc05 > src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 > src/tests/authorization_tests.cpp 9d046e8d53cbb6c065a23ca3f7832021ec7faadc > src/tests/persistent_volume_endpoints_tests.cpp > 6069ca1e9ed278459c5182e438417e95955b1924 > src/tests/persistent_volume_tests.cpp > e169e1b141a38dc389eefd42c11a078c413123d5 > > Diff: https://reviews.apache.org/r/43782/diff/ > > > Testing > --- > > Persistent volume tests and operation validation tests were altered to > accomodate the new ACL object, and some new principal/role combinations were > added to `AuthorizationTest.CreateVolume`. > > Ran `configure && make check` and `configure --enable-libevent --enable-ssl > && make check` on OSX; all tests passed. > > > Thanks, > > Greg Mann > >
Re: Review Request 43798: Added overview section to upgrades.md.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43798/#review120924 --- Ship it! Basic impression: this isn't ideal, but I'm not sure we can do better given the formatting constraints of Markdown. So, ship it I guess? :) - Neil Conway On Feb. 23, 2016, 9:39 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43798/ > --- > > (Updated Feb. 23, 2016, 9:39 p.m.) > > > Review request for mesos, Michael Park, Neil Conway, and Till Toenshoff. > > > Bugs: MESOS-4381 > https://issues.apache.org/jira/browse/MESOS-4381 > > > Repository: mesos > > > Description > --- > > Added overview section to upgrades.md. > > > Diffs > - > > docs/upgrades.md 4f30d725c6ed28c09a1c5528fd4193c3f06b2d93 > > Diff: https://reviews.apache.org/r/43798/diff/ > > > Testing > --- > > Viewed via gist (https://gist.github.com/joerg84/eddbc0302a5a4b291e81) and > docker website container. > > > Thanks, > > Joerg Schad > >
Re: Review Request 44047: Added full reserved resource info to `/slaves` master endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44047/ --- (Updated Feb. 26, 2016, 7:10 p.m.) Review request for mesos, Michael Park and Vinod Kone. Changes --- Tweak docs. Bugs: MESOS-4667 https://issues.apache.org/jira/browse/MESOS-4667 Repository: mesos Description --- This allows operators to list all the dynamic reservations and persistent volumes in a cluster. This is important in itself; it also makes it easier to use the `/unreserve` and `/destroy-volumes` endpoints. Diffs (updated) - docs/persistent-volume.md 2a794a572ff930aa1f95706b89fef9243be627de docs/reservation.md b98ebe6df0739b48c5fa58e087fd64b1c6c5d456 src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 src/tests/persistent_volume_endpoints_tests.cpp 6069ca1e9ed278459c5182e438417e95955b1924 Diff: https://reviews.apache.org/r/44047/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43796: Added documentation for `cgroups/net_cls` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43796/#review120950 --- Ship it! Ship It! - Neil Conway On Feb. 22, 2016, 6:26 p.m., Avinash sridharan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43796/ > --- > > (Updated Feb. 22, 2016, 6:26 p.m.) > > > Review request for mesos, Jie Yu and Neil Conway. > > > Bugs: MESOS-4660 > https://issues.apache.org/jira/browse/MESOS-4660 > > > Repository: mesos > > > Description > --- > > Added documentation for `cgroups/net_cls` isolator. > > > Diffs > - > > docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 > > Diff: https://reviews.apache.org/r/43796/diff/ > > > Testing > --- > > Built the web-site using docker, and proof read the website and links on > localhost. > > Verified all the links embedded in markdown work. > > > Thanks, > > Avinash sridharan > >
Review Request 44101: Added links to the operator endpoint doc pages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44101/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Added links to the operator endpoint doc pages. Diffs - docs/logging.md 50a95ba5fedcb5b803e183bea8243e2e7750aa3e docs/mesos-containerizer.md 87f145cd957dcb8fd3188c866212b417f0ab6296 docs/network-monitoring.md 57b859e9292dc22613e235710312d1a33f58e0c0 docs/networking-for-mesos-managed-containers.md f2fbea54cc3a7f1727c53629c86feb25d7f5fd68 docs/persistent-volume.md f772405ca9a27eb47b1f0aad9f54df5b47e88cbc docs/quota.md 48dcd2a79202a6d5fbee86c6ae458cbdd71a22ad docs/reservation.md cbf0a085dfe4e514aede11322f6025c7ff376207 docs/roles.md 84e5b7eaa4cf5cd3e7ebd6151928210be93d1d56 docs/sandbox.md 276e1126f495e7af15afd4f4ad8c63beb40db739 Diff: https://reviews.apache.org/r/44101/diff/ Testing --- Previewed links in site-docker and verified that they work. Thanks, Neil Conway
Re: Review Request 43938: Required jsonifying of generic protobuf to be explicit opt-in [stout].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43938/#review120963 --- Fix it, then Ship it! 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 624) <https://reviews.apache.org/r/43938/#comment182519> This needs a comment, I think. 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 625) <https://reviews.apache.org/r/43938/#comment182520> Is this the best name for this type? Not sure there's a better name, but `JSON::Protobuf` doesn't necessarily imply to me that the type names a certain representation of `google::protobuf::Message`. What about `JSON::RawProtobuf`? - Neil Conway On Feb. 26, 2016, 8:39 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43938/ > --- > > (Updated Feb. 26, 2016, 8:39 p.m.) > > > Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil > Conway. > > > Bugs: MESOS-4754 > https://issues.apache.org/jira/browse/MESOS-4754 > > > Repository: mesos > > > Description > --- > > See JIRA ticket. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp > eb5502c4987da5593169a86b21f60c01aa5b5170 > 3rdparty/libprocess/3rdparty/stout/include/stout/representation.hpp > 22f70f7536c6f5d24ff59228d8ba7bf41319fd4a > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp > 8dd9cfd3e7d1e3ab4ace87066a43a3094b776d82 > > Diff: https://reviews.apache.org/r/43938/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 43939: Required jsonifying of generic protobuf to be explicit opt-in [mesos].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43939/#review120965 --- Ship it! Ship It! - Neil Conway On Feb. 26, 2016, 8:39 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43939/ > --- > > (Updated Feb. 26, 2016, 8:39 p.m.) > > > Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Neil > Conway. > > > Bugs: MESOS-4754 > https://issues.apache.org/jira/browse/MESOS-4754 > > > Repository: mesos > > > Description > --- > > See JIRA ticket. > > > Diffs > - > > src/common/http.cpp f8a0441b35898ae31eb5d5248c710847be3ab37a > src/master/http.cpp 939fab21a2240de7214ef809a194ffb3837a9f1b > > Diff: https://reviews.apache.org/r/43939/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
> 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. > > Neil Conway wrote: > 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-)... > > Klaus Ma wrote: > `CHECK` or warning log should be fine, it only improves the debugability; > it different with `operator+` because `max_double/2` should be big enough for > us; but for `double * 1000 -> long`, I'm not sure of that. > > And I think we need to use `long long` or `int64` instead of `long`; > `long` is not garantee to be 64bit in c++. > > Neil Conway wrote: > re: `long`, good point -- we only officially support 64-bit OSX, Linux, > and Windows, but `long` is 32-bit on Windows. After discussing this with a few folks offline, our feeling was that the range of resource values we can represent without overflow should be sufficient for any reasonable purpose for a considerable length of time. Hopefully we'll have switched to a fixed point format before people use Mesos to manage > 10s of exabytes as a single resource :) - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review119382 --- On Feb. 19, 2016, 10:27 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43635/ > --- > > (Updated Feb. 19, 2016, 10:27 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 636550f255a122d7f714dbd58f587bea8221b461 > include/mesos/v1/mesos.proto 1d5af88a343fe9d81688bb3e83aa997ccba7d030 > src/common/resources.cpp 5d731870542166cec11f9956ccdf16207b2d22cc > src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 > src/master/allocator/mesos/hierarchical.cpp > 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d > 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
Review Request 44113: Cleaned up assertions in test cases.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44113/ --- Review request for mesos, Joris Van Remoortere and Michael Park. Repository: mesos Description --- Ensure that `EXPECT_EQ` and friends use "expected, actual" as the order of arguments. Diffs - src/tests/containerizer/docker_spec_tests.cpp 5799dc9c871c6ccaddb209ab7ec1112b192f3d41 src/tests/containerizer/docker_tests.cpp 620819330847a10d9dcd817968df9d2b180a9a29 src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb src/tests/containerizer/provisioner_backend_tests.cpp 25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 src/tests/module_tests.cpp 121d65337bdf29f6049ac44bfd903a1f5ea1a09d src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff src/tests/sorter_tests.cpp 22bc618f9b4958fbd8e6c5dfee94b26fe13ec47a Diff: https://reviews.apache.org/r/44113/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 26, 2016, 11:52 p.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address code review comments. 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 (updated) - docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 src/master/allocator/mesos/hierarchical.cpp 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 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. PERFORMANCE: This is the performance of the `DeclineOffers` benchmark WITHOUT this RR applied (optimized build on my laptop): ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.192425secs to make 200 offers round 1 allocate took 2.221243secs to make 200 offers round 2 allocate took 2.236314secs to make 200 offers round 3 allocate took 2.224045secs to make 200 offers round 4 allocate took 2.232822secs to make 200 offers round 5 allocate took 2.264807secs to make 200 offers round 6 allocate took 2.224853secs to make 200 offers round 7 allocate took 2.224829secs to make 200 offers round 8 allocate took 2.24862secs to make 200 offers round 9 allocate took 2.2556secs to make 200 offers round 10 allocate took 2.256616secs to make 200 offers ``` And after applying this RR: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.267919secs to make 200 offers round 1 allocate took 2.202843secs to make 200 offers round 2 allocate took 2.20426secs to make 200 offers round 3 allocate took 2.263887secs to make 200 offers round 4 allocate took 2.266237secs to make 200 offers round 5 allocate took 2.276957secs to make 200 offers round 6 allocate took 2.291821secs to make 200 offers round 7 allocate took 2.261839secs to make 200 offers round 8 allocate took 2.325696secs to make 200 offers round 9 allocate took 2.310469secs to make 200 offers round 10 allocate took 2.21802secs to make 200 offers ``` Which suggests to me that any performance hit is pretty minimal. Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
> On Feb. 26, 2016, 9:40 p.m., Joris Van Remoortere wrote: > > src/common/values.cpp, line 61 > > <https://reviews.apache.org/r/43635/diff/4/?file=1263289#file1263289line61> > > > > let's pull out into a constant as discussed offline. Per discussion, we aren't going to do this for now (stupid `constexpr`). - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/#review120961 --- On Feb. 26, 2016, 11:52 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43635/ > --- > > (Updated Feb. 26, 2016, 11:52 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 21faea8a3c152b15023d6fa69cde9382dac80c18 > include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 > include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 > src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 > src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 > src/master/allocator/mesos/hierarchical.cpp > 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d > src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff > src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d > 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. > > PERFORMANCE: > > This is the performance of the `DeclineOffers` benchmark WITHOUT this RR > applied (optimized build on my laptop): > > ``` > [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers > Using 2000 slaves and 200 frameworks > round 0 allocate took 2.192425secs to make 200 offers > round 1 allocate took 2.221243secs to make 200 offers > round 2 allocate took 2.236314secs to make 200 offers > round 3 allocate took 2.224045secs to make 200 offers > round 4 allocate took 2.232822secs to make 200 offers > round 5 allocate took 2.264807secs to make 200 offers > round 6 allocate took 2.224853secs to make 200 offers > round 7 allocate took 2.224829secs to make 200 offers > round 8 allocate took 2.24862secs to make 200 offers > round 9 allocate took 2.2556secs to make 200 offers > round 10 allocate took 2.256616secs to make 200 offers > ``` > > And after applying this RR: > > ``` > [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers > Using 2000 slaves and 200 frameworks > round 0 allocate took 2.2
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 27, 2016, 12:10 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Add comments. 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 (updated) - docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 src/master/allocator/mesos/hierarchical.cpp 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 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. PERFORMANCE: This is the performance of the `DeclineOffers` benchmark WITHOUT this RR applied (optimized build on my laptop): ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.192425secs to make 200 offers round 1 allocate took 2.221243secs to make 200 offers round 2 allocate took 2.236314secs to make 200 offers round 3 allocate took 2.224045secs to make 200 offers round 4 allocate took 2.232822secs to make 200 offers round 5 allocate took 2.264807secs to make 200 offers round 6 allocate took 2.224853secs to make 200 offers round 7 allocate took 2.224829secs to make 200 offers round 8 allocate took 2.24862secs to make 200 offers round 9 allocate took 2.2556secs to make 200 offers round 10 allocate took 2.256616secs to make 200 offers ``` And after applying this RR: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.267919secs to make 200 offers round 1 allocate took 2.202843secs to make 200 offers round 2 allocate took 2.20426secs to make 200 offers round 3 allocate took 2.263887secs to make 200 offers round 4 allocate took 2.266237secs to make 200 offers round 5 allocate took 2.276957secs to make 200 offers round 6 allocate took 2.291821secs to make 200 offers round 7 allocate took 2.261839secs to make 200 offers round 8 allocate took 2.325696secs to make 200 offers round 9 allocate took 2.310469secs to make 200 offers round 10 allocate took 2.21802secs to make 200 offers ``` Which suggests to me that any performance hit is pretty minimal. Thanks, Neil Conway
Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44110/#review121017 --- docs/configuration.md (line 86) <https://reviews.apache.org/r/44110/#comment182602> Is this a valid endpoint? "/roles" is a master endpoint but not an agent endpoint AFAIK. - Neil Conway On Feb. 27, 2016, 12:33 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44110/ > --- > > (Updated Feb. 27, 2016, 12:33 a.m.) > > > Review request for mesos, Alexander Rojas and Vinod Kone. > > > Bugs: MESOS-4509 > https://issues.apache.org/jira/browse/MESOS-4509 > > > Repository: mesos > > > Description > --- > > Updated flag examples to refer to /role instead of stats.json. > > > Diffs > - > > docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 > src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d > src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d > > Diff: https://reviews.apache.org/r/44110/diff/ > > > Testing > --- > > make test + manually checked the flags example. > > > Thanks, > > Joerg Schad > >
Re: Review Request 44110: Updated flag examples to refer to /role instead of stats.json.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44110/#review121025 --- Ship it! Ship It! - Neil Conway On Feb. 27, 2016, 12:55 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44110/ > --- > > (Updated Feb. 27, 2016, 12:55 a.m.) > > > Review request for mesos, Alexander Rojas and Vinod Kone. > > > Bugs: MESOS-4509 > https://issues.apache.org/jira/browse/MESOS-4509 > > > Repository: mesos > > > Description > --- > > Updated flag examples to refer to /role instead of stats.json. > > > Diffs > - > > docs/configuration.md 2353e78a80548b63f871c52e840ffe2fe869f4d7 > src/master/flags.cpp 60e085bd5c6689adb625a736edc76e814860ea7d > src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d > > Diff: https://reviews.apache.org/r/44110/diff/ > > > Testing > --- > > make test + manually checked the flags example. > > > Thanks, > > Joerg Schad > >
Re: Review Request 44113: Cleaned up assertions in test cases.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44113/ --- (Updated Feb. 27, 2016, 1:56 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Rebase. Repository: mesos Description --- Ensure that `EXPECT_EQ` and friends use "expected, actual" as the order of arguments. Diffs (updated) - src/tests/containerizer/docker_spec_tests.cpp 5799dc9c871c6ccaddb209ab7ec1112b192f3d41 src/tests/containerizer/docker_tests.cpp 620819330847a10d9dcd817968df9d2b180a9a29 src/tests/containerizer/fs_tests.cpp 29e43877612fa151e6f6d79268a7411272a7bfeb src/tests/containerizer/provisioner_backend_tests.cpp 25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 src/tests/module_tests.cpp 121d65337bdf29f6049ac44bfd903a1f5ea1a09d src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff src/tests/sorter_tests.cpp 22bc618f9b4958fbd8e6c5dfee94b26fe13ec47a Diff: https://reviews.apache.org/r/44113/diff/ Testing --- make check Thanks, Neil Conway
Review Request 44126: Fixed a few style issues in the docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44126/ --- Review request for mesos, Joris Van Remoortere and Michael Park. Repository: mesos Description --- Fixed a few style issues in the docs. Diffs - docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e Diff: https://reviews.apache.org/r/44126/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 43635: Changed scalar resources to use fixed-point internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43635/ --- (Updated Feb. 27, 2016, 1:57 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address review comments. 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 (updated) - docs/attributes-resources.md 818da8ab0c672144b02f526b2b805cf0505d2c7e docs/upgrades.md 21faea8a3c152b15023d6fa69cde9382dac80c18 include/mesos/mesos.proto 33f6b0838360b61db70a247e5d6dfb16af15aa06 include/mesos/v1/mesos.proto 1b0e709e76f3f6b44ab0434c649c064e8866c8a1 src/common/resources.cpp 529a1cd99707f8ce7bcc22889793d1eea04c3338 src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 src/master/allocator/mesos/hierarchical.cpp 5ef29f26ec8071f79c2f4f78dbe2bb0a613cc92d src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff src/v1/resources.cpp 49dc3429f3b4b18e24f80052ed8be830df53b59d 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. PERFORMANCE: This is the performance of the `DeclineOffers` benchmark WITHOUT this RR applied (optimized build on my laptop): ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.192425secs to make 200 offers round 1 allocate took 2.221243secs to make 200 offers round 2 allocate took 2.236314secs to make 200 offers round 3 allocate took 2.224045secs to make 200 offers round 4 allocate took 2.232822secs to make 200 offers round 5 allocate took 2.264807secs to make 200 offers round 6 allocate took 2.224853secs to make 200 offers round 7 allocate took 2.224829secs to make 200 offers round 8 allocate took 2.24862secs to make 200 offers round 9 allocate took 2.2556secs to make 200 offers round 10 allocate took 2.256616secs to make 200 offers ``` And after applying this RR: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.267919secs to make 200 offers round 1 allocate took 2.202843secs to make 200 offers round 2 allocate took 2.20426secs to make 200 offers round 3 allocate took 2.263887secs to make 200 offers round 4 allocate took 2.266237secs to make 200 offers round 5 allocate took 2.276957secs to make 200 offers round 6 allocate took 2.291821secs to make 200 offers round 7 allocate took 2.261839secs to make 200 offers round 8 allocate took 2.325696secs to make 200 offers round 9 allocate took 2.310469secs to make 200 offers round 10 allocate took 2.21802secs to make 200 offers ``` Which suggests to me that any performance hit is pretty minimal. Thanks, Neil Conway
Re: Review Request 43636: Cleaned up various code in a test file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43636/ --- (Updated Feb. 27, 2016, 1:58 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Repository: mesos Description --- Cleaned up various code in a test file. Diffs (updated) - src/tests/teardown_tests.cpp 5753559003d703138d2bbee6a1ac93473ba0b0c0 Diff: https://reviews.apache.org/r/43636/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 44144: Improved the documentation for setting ACLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44144/#review121096 --- Ship it! Ship It! - Neil Conway On Feb. 27, 2016, 5:49 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44144/ > --- > > (Updated Feb. 27, 2016, 5:49 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Neil Conway. > > > Repository: mesos > > > Description > --- > > Improved the documentation for setting ACLs. > > > Diffs > - > > docs/authorization.md feb0ddd6514106f0778db7b5428e47d4d8dc4d42 > > Diff: https://reviews.apache.org/r/44144/diff/ > > > Testing > --- > > Checked via gist: https://gist.github.com/joerg84/6c2a86e1b0c378c4c8c9 > > > Thanks, > > Joerg Schad > >
Re: Review Request 43910: Enhanced a test case for the `/state` agent endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43910/ --- (Updated Feb. 28, 2016, 12:15 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Enhanced a test case for the `/state` agent endpoint. Diffs (updated) - src/tests/slave_tests.cpp 322f3ddaf11885d7e61e0e9232c0342e97d8bfa1 Diff: https://reviews.apache.org/r/43910/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43816: Updated `/frameworks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43816/ --- (Updated Feb. 28, 2016, 12:15 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- Updated `/frameworks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp f3ce1aa22f5f753fcb254e9ecaa8ba571e3d2829 Diff: https://reviews.apache.org/r/43816/diff/ Testing --- 1. make check 2. Verified that after introducing a bug into the jsonify version of `frameworks()`, `make check` fails (i.e., the test suite covers the `/frameworks` endpoint). 3. Compared output of `/frameworks` with old and new implementation on a test Mesos installation to try to gauge correctness visually. Thanks, Neil Conway
Re: Review Request 43822: Updated `/slaves` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43822/ --- (Updated Feb. 28, 2016, 12:15 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/slaves` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp f3ce1aa22f5f753fcb254e9ecaa8ba571e3d2829 Diff: https://reviews.apache.org/r/43822/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Re: Review Request 43823: Updated `/tasks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43823/ --- (Updated Feb. 28, 2016, 12:15 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/tasks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp f3ce1aa22f5f753fcb254e9ecaa8ba571e3d2829 Diff: https://reviews.apache.org/r/43823/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
Re: Review Request 43911: Updated `/state` agent endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43911/ --- (Updated Feb. 28, 2016, 12:16 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/state` agent endpoint to use jsonify. Diffs (updated) - src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e Diff: https://reviews.apache.org/r/43911/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 44047: Added full reserved resource info to `/slaves` master endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44047/ --- (Updated Feb. 28, 2016, 12:17 a.m.) Review request for mesos, Michael Park and Vinod Kone. Changes --- Rebase. Bugs: MESOS-4667 https://issues.apache.org/jira/browse/MESOS-4667 Repository: mesos Description --- This allows operators to list all the dynamic reservations and persistent volumes in a cluster. This is important in itself; it also makes it easier to use the `/unreserve` and `/destroy-volumes` endpoints. Diffs (updated) - docs/persistent-volume.md 47ada98413f1670e9fc4ebd9d1ead6af9b120184 docs/reservation.md 450f4eec49d957b096df1380c3e79d5f743cc829 src/master/http.cpp f3ce1aa22f5f753fcb254e9ecaa8ba571e3d2829 src/tests/persistent_volume_endpoints_tests.cpp 08b9102318b826bab9d2c1d389fb80b86949218c Diff: https://reviews.apache.org/r/44047/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43817: Removed no-longer-used model functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43817/ --- (Updated Feb. 28, 2016, 12:17 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Bugs: MESOS-4731 https://issues.apache.org/jira/browse/MESOS-4731 Repository: mesos Description --- These `model()` variants provided functionality that is now implemented via jsonify; no more call-sites of the old functions remain. Diffs (updated) - src/master/http.cpp f3ce1aa22f5f753fcb254e9ecaa8ba571e3d2829 Diff: https://reviews.apache.org/r/43817/diff/ Testing --- make check Also updated a few comments to not refer to `summarize`. Thanks, Neil Conway
Re: Review Request 43910: Enhanced a test case for the `/state` agent endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43910/ --- (Updated Feb. 28, 2016, 12:21 a.m.) Review request for mesos and Michael Park. Repository: mesos Description --- Enhanced a test case for the `/state` agent endpoint. Diffs - src/tests/slave_tests.cpp 322f3ddaf11885d7e61e0e9232c0342e97d8bfa1 Diff: https://reviews.apache.org/r/43910/diff/ Testing (updated) --- make check Note that we don't currently check the conversion of `TaskInfo` -> JSON, which is used for `queuedTasks`. Would be nice to improve the test case so that the slave has a queued task, although this will probably require some `DROP_MESSAGE` trickery... Thanks, Neil Conway
Re: Review Request 44147: Remove unused src/common/date_utils.{c, h}pp (MESOS-4792).
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44147/#review121168
---
Ship it!
Ship It!
- Neil Conway
On Feb. 28, 2016, 7:36 p.m., Yong Tang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44147/
> ---
>
> (Updated Feb. 28, 2016, 7:36 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-4792
> https://issues.apache.org/jira/browse/MESOS-4792
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unused src/common/date_utils.{c,h}pp (MESOS-4792).
>
>
> Diffs
> -
>
> src/CMakeLists.txt 49a5645ef7242dbaee31e7b26dbbcb1f4f1f910e
> src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5
> src/common/date_utils.hpp 794510c0f31eab6dafe8b86d835710096aac3392
> src/common/date_utils.cpp 64eb33fee0c6155db3d6bcd3086bfcf4d7d9f7c0
> src/master/master.cpp 7c62f2a882a1c89d73f328b2ae665422fd84d7a1
>
> Diff: https://reviews.apache.org/r/44147/diff/
>
>
> Testing
> ---
>
> make
> make check
>
>
> Thanks,
>
> Yong Tang
>
>
Re: Review Request 44047: Added full reserved resource info to `/slaves` master endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44047/ --- (Updated Feb. 29, 2016, 6:55 a.m.) Review request for mesos, Michael Park and Vinod Kone. Changes --- Rebase. Bugs: MESOS-4667 https://issues.apache.org/jira/browse/MESOS-4667 Repository: mesos Description --- This allows operators to list all the dynamic reservations and persistent volumes in a cluster. This is important in itself; it also makes it easier to use the `/unreserve` and `/destroy-volumes` endpoints. Diffs (updated) - docs/persistent-volume.md 47ada98413f1670e9fc4ebd9d1ead6af9b120184 docs/reservation.md 450f4eec49d957b096df1380c3e79d5f743cc829 src/master/http.cpp d6e1f22620dfc4271244a2983195cffc36da6e8e src/tests/persistent_volume_endpoints_tests.cpp 08b9102318b826bab9d2c1d389fb80b86949218c Diff: https://reviews.apache.org/r/44047/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43911: Updated `/state` agent endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43911/ --- (Updated Feb. 29, 2016, 6:55 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/state` agent endpoint to use jsonify. Diffs (updated) - src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e Diff: https://reviews.apache.org/r/43911/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43910: Enhanced a test case for the `/state` agent endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43910/ --- (Updated Feb. 29, 2016, 6:55 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Enhanced a test case for the `/state` agent endpoint. Diffs (updated) - src/tests/slave_tests.cpp 322f3ddaf11885d7e61e0e9232c0342e97d8bfa1 Diff: https://reviews.apache.org/r/43910/diff/ Testing --- make check Note that we don't currently check the conversion of `TaskInfo` -> JSON, which is used for `queuedTasks`. Would be nice to improve the test case so that the slave has a queued task, although this will probably require some `DROP_MESSAGE` trickery... Thanks, Neil Conway
Re: Review Request 43823: Updated `/tasks` master endpoint to use jsonify.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43823/ --- (Updated Feb. 29, 2016, 6:55 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Updated `/tasks` master endpoint to use jsonify. Diffs (updated) - src/master/http.cpp d6e1f22620dfc4271244a2983195cffc36da6e8e Diff: https://reviews.apache.org/r/43823/diff/ Testing --- make check. Also verified that this endpoint is covered by the unit tests. Thanks, Neil Conway
