You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/12/13 06:57:06 UTC
Review Request 29018: Added abstraction for resources transformation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030
Repository: mesos-git
Description
-------
Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
Diffs
-----
include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
Diff: https://reviews.apache.org/r/29018/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 211-214
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line211>
> >
> > To keep consistent with the rest of the code base, couldn't you have done the following?
> >
> > ```
> > template <typename T>
> > void add(const T& t)
> > {
> > transformations.push_back(new T(t));
> > }
> > ```
> >
> > With unique_ptr, we're requiring that ownership be transferred if the caller is holding the transformation, which seems a bit odd for our library to impose on the caller:
> >
> > ```
> > // The caller cannot keep ownership of t.
> > std::unique_ptr<Transformation> t(new DiskTransformation(...));
> > composite.add(std::move(t));
> >
> > // Ok.
> > composite.add(std::unique_ptr<Transformation>(new DiskTransformation(...));
> > ```
> >
> > Could we keep the const argument semantics for now for consistency? Then the caller doesn't have to use 'new' as well. Thoughts?
>
> Dominic Hamon wrote:
> That's exactly the point of std::unique_ptr - a library is finally able to inform the caller that they're taking responsibility for the object and the memory associated with it. I don't see that as odd.
>
> Can you describe a use case where a caller might want to retain a reference to the transformation?
>
> Jie Yu wrote:
> OK, since we haven't finalized the unique_ptr usage in the style guide, I'll use your suggestion and add a TODO here.
Thanks Jie and Dominic! This could very well be a great place to leverage unique_ptr, but as jie said let's take this example to the thread :)
The point that we'll want to consider there is that function calls with value semantics are generally easier to reason about:
```cpp
CompositeTransformation composite;
composite.add(DiskTransformation(disk));
FooBarTransformation transformation; ...;
composite.add(transformation);
```
The caller here only has to reason about values. The callee is responsible for taking a copy if it's needed.
Here we have to reason about memory ownership:
```cpp
CompositeTransformation composite;
composite.add(unique_ptr<Transformation>(new DiskTransformation(disk)));
unique_ptr<Transformation> transformation(new FooBarTransformation()); ...;
composite.add(std::move(transformation));
// Also consider:
transformation->...; // Yikes! As far as I can tell, the compiler won't warn about this?
```
Another point to bring up in the summary from that thread (as it's getting a bit long), is whether compilers warn about using moved objects, doesn't seem like it?
http://stackoverflow.com/questions/14552690/warn-if-accessing-moved-object-in-c11
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
-----------------------------------------------------------
On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 9:10 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 15, 2014, 1:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 211-214
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line211>
> >
> > To keep consistent with the rest of the code base, couldn't you have done the following?
> >
> > ```
> > template <typename T>
> > void add(const T& t)
> > {
> > transformations.push_back(new T(t));
> > }
> > ```
> >
> > With unique_ptr, we're requiring that ownership be transferred if the caller is holding the transformation, which seems a bit odd for our library to impose on the caller:
> >
> > ```
> > // The caller cannot keep ownership of t.
> > std::unique_ptr<Transformation> t(new DiskTransformation(...));
> > composite.add(std::move(t));
> >
> > // Ok.
> > composite.add(std::unique_ptr<Transformation>(new DiskTransformation(...));
> > ```
> >
> > Could we keep the const argument semantics for now for consistency? Then the caller doesn't have to use 'new' as well. Thoughts?
That's exactly the point of std::unique_ptr - a library is finally able to inform the caller that they're taking responsibility for the object and the memory associated with it. I don't see that as odd.
Can you describe a use case where a caller might want to retain a reference to the transformation?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
-----------------------------------------------------------
On Dec. 12, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 9:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 191-202
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line191>
> >
> > Great, do we want to specify what a valid transformation is? In particular, it seems like the invariant of a Transformation is that the amount of a resource ("cpus", "mem", "disk", "ports", ...) remains unchanged and only the "metadata" of the resources can be manipulated. Let's document that this is what a Transformation is?
> >
> > For invariants, maybe just a NOTE for now.. but I'd love to have invariants enforced in the allocator (if not provided by Transformation the allocator will have to manually check them).
> >
> > One suggestion is, similary to the Registrar's Operation, we can keep a top level `apply` or function operator as non-virtual, which performs the transformation and validates any of the invariants (cpus, mem, disk, ports remain the same) and rely on a virtual method to perform the actual transformation? That way, Transformation can enforce the invariants, and a CHECK fails or an Error results if they are broken.
Added a non virtual operator which calls the virtual apply. Enforced the invariants in the operator.
> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 211-214
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line211>
> >
> > To keep consistent with the rest of the code base, couldn't you have done the following?
> >
> > ```
> > template <typename T>
> > void add(const T& t)
> > {
> > transformations.push_back(new T(t));
> > }
> > ```
> >
> > With unique_ptr, we're requiring that ownership be transferred if the caller is holding the transformation, which seems a bit odd for our library to impose on the caller:
> >
> > ```
> > // The caller cannot keep ownership of t.
> > std::unique_ptr<Transformation> t(new DiskTransformation(...));
> > composite.add(std::move(t));
> >
> > // Ok.
> > composite.add(std::unique_ptr<Transformation>(new DiskTransformation(...));
> > ```
> >
> > Could we keep the const argument semantics for now for consistency? Then the caller doesn't have to use 'new' as well. Thoughts?
>
> Dominic Hamon wrote:
> That's exactly the point of std::unique_ptr - a library is finally able to inform the caller that they're taking responsibility for the object and the memory associated with it. I don't see that as odd.
>
> Can you describe a use case where a caller might want to retain a reference to the transformation?
OK, since we haven't finalized the unique_ptr usage in the style guide, I'll use your suggestion and add a TODO here.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
-----------------------------------------------------------
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
-----------------------------------------------------------
Thanks Jie!
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108078>
Great, do we want to specify what a valid transformation is? In particular, it seems like the invariant of a Transformation is that the amount of a resource ("cpus", "mem", "disk", "ports", ...) remains unchanged and only the "metadata" of the resources can be manipulated. Let's document that this is what a Transformation is?
For invariants, maybe just a NOTE for now.. but I'd love to have invariants enforced in the allocator (if not provided by Transformation the allocator will have to manually check them).
One suggestion is, similary to the Registrar's Operation, we can keep a top level `apply` or function operator as non-virtual, which performs the transformation and validates any of the invariants (cpus, mem, disk, ports remain the same) and rely on a virtual method to perform the actual transformation? That way, Transformation can enforce the invariants, and a CHECK fails or an Error results if they are broken.
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108076>
To keep consistent with the rest of the code base, couldn't you have done the following?
```
template <typename T>
void add(const T& t)
{
transformations.push_back(new T(t));
}
```
With unique_ptr, we're requiring that ownership be transferred if the caller is holding the transformation, which seems a bit odd for our library to impose on the caller:
```
// The caller cannot keep ownership of t.
std::unique_ptr<Transformation> t(new DiskTransformation(...));
composite.add(std::move(t));
// Ok.
composite.add(std::unique_ptr<Transformation>(new DiskTransformation(...));
```
Could we keep the const argument semantics for now for consistency? Then the caller doesn't have to use 'new' as well. Thoughts?
- Ben Mahler
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 190-198
> > <https://reviews.apache.org/r/29018/diff/2/?file=793220#file793220line190>
> >
> > Maybe this could be a bit more succinct:
> >
> > ```
> > // This is an abstraction for describing a transformation that can
> > // be applied to Resources. Transformations cannot not alter the
> > // quantity, or the static role of the resources.
> > //
> > // TODO(jieyu): Enforce the transformation invariants fully!
> > class Transformation
> > {
> > public:
> > ...
> >
> > // Returns the result of the transformation, applied to 'resources'.
> > // Returns an Error if the transformation cannot be applied.
> > Try<Resources> operator () (const Resources& resources) const;
> > };
> > ```
Done.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 210-212
> > <https://reviews.apache.org/r/29018/diff/2/?file=793220#file793220line210>
> >
> > Maybe mention the all-or-nothing nature:
> >
> > ```
> > // Represents a sequence of transformations, the transformations
> > // are applied in an all-or-nothing manner.
> > class CompositeTransformation : public Transformation
> > {
> > };
> > ```
Done.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 236
> > <https://reviews.apache.org/r/29018/diff/2/?file=793220#file793220line236>
> >
> > You need an include for vector now?
DOne.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 33
> > <https://reviews.apache.org/r/29018/diff/2/?file=793221#file793221line33>
> >
> > Unused?
Removed.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 860
> > <https://reviews.apache.org/r/29018/diff/2/?file=793221#file793221line860>
> >
> > "transformations"?
Fixed.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 864-885
> > <https://reviews.apache.org/r/29018/diff/2/?file=793221#file793221line864>
> >
> > Thanks, could this be:
> >
> > ```
> > Try<Resources> applied = apply(resources);
> >
> > if (applied.isSome()) {
> > // Additional checks.
> > }
> >
> > return applied;
> > ```
Done.
> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 895
> > <https://reviews.apache.org/r/29018/diff/2/?file=793221#file793221line895>
> >
> > newline here?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65241
-----------------------------------------------------------
On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 9:10 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65241
-----------------------------------------------------------
Ship it!
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108282>
Maybe this could be a bit more succinct:
```
// This is an abstraction for describing a transformation that can
// be applied to Resources. Transformations cannot not alter the
// quantity, or the static role of the resources.
//
// TODO(jieyu): Enforce the transformation invariants fully!
class Transformation
{
public:
...
// Returns the result of the transformation, applied to 'resources'.
// Returns an Error if the transformation cannot be applied.
Try<Resources> operator () (const Resources& resources) const;
};
```
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108283>
Maybe mention the all-or-nothing nature:
```
// Represents a sequence of transformations, the transformations
// are applied in an all-or-nothing manner.
class CompositeTransformation : public Transformation
{
};
```
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108287>
You need an include for vector now?
src/common/resources.cpp
<https://reviews.apache.org/r/29018/#comment108286>
Unused?
src/common/resources.cpp
<https://reviews.apache.org/r/29018/#comment108285>
"transformations"?
src/common/resources.cpp
<https://reviews.apache.org/r/29018/#comment108280>
Thanks, could this be:
```
Try<Resources> applied = apply(resources);
if (applied.isSome()) {
// Additional checks.
}
return applied;
```
src/common/resources.cpp
<https://reviews.apache.org/r/29018/#comment108281>
newline here?
- Ben Mahler
On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 9:10 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/
-----------------------------------------------------------
(Updated Dec. 16, 2014, 9:10 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Review comments.
Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030
Repository: mesos-git
Description
-------
Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
Diffs (updated)
-----
include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
Diff: https://reviews.apache.org/r/29018/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Michael Park <mc...@gmail.com>.
> On Dec. 15, 2014, 6:08 p.m., Dominic Hamon wrote:
> > include/mesos/resources.hpp, line 213
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line213>
> >
> > transformations.emplace_back (if we have it in 4.4) might save you the explicit move call, and will certainly save you the extra construction.
Given `std::vector<T>`, `emplace_back` and `push_back` are equivalent when called with a single argument of type `T`, whether rvalue or lvalue.
The times `emplace_back` wins the race is when we save a move construction.
For example, given `vector<std::string> v;` `v.emplace_back("foo")` is preferred over `v.push_back("foo")` since `emplace_back` saves a move construction.
In this case however, you don't save anything by using `emplace_back`.
(Of course, unless I'm missing something)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65096
-----------------------------------------------------------
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65096
-----------------------------------------------------------
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108041>
transformations.emplace_back (if we have it in 4.4) might save you the explicit move call, and will certainly save you the extra construction.
- Dominic Hamon
On Dec. 12, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 9:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65156
-----------------------------------------------------------
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108146>
Applying a transformation should be a const operation, right?
- Ben Mahler
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Michael Park <mc...@gmail.com>.
> On Dec. 15, 2014, 9:47 p.m., Dominic Hamon wrote:
> > include/mesos/resources.hpp, line 213
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line213>
> >
> > not quite. emplace_back with a unique_ptr works because unique_ptr doesn't have a copy constructor. however, as it turns out, both push_back(std::move(..)) and emplace_back(..) may leak if the vector grow fails and an exception is thrown.
> >
> > ideally, then, this should be push_back(make_unique<>(..))
> >
> > but we don't have make_unique ;)
Hey Dominic, was this supposed to be a reply to my comment?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65125
-----------------------------------------------------------
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 15, 2014, 1:47 p.m., Dominic Hamon wrote:
> > include/mesos/resources.hpp, line 213
> > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line213>
> >
> > not quite. emplace_back with a unique_ptr works because unique_ptr doesn't have a copy constructor. however, as it turns out, both push_back(std::move(..)) and emplace_back(..) may leak if the vector grow fails and an exception is thrown.
> >
> > ideally, then, this should be push_back(make_unique<>(..))
> >
> > but we don't have make_unique ;)
>
> Michael Park wrote:
> Hey Dominic, was this supposed to be a reply to my comment?
yes, sorry. i forgot that it doesn't thread correctly from the diff.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65125
-----------------------------------------------------------
On Dec. 12, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 9:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65125
-----------------------------------------------------------
include/mesos/resources.hpp
<https://reviews.apache.org/r/29018/#comment108083>
not quite. emplace_back with a unique_ptr works because unique_ptr doesn't have a copy constructor. however, as it turns out, both push_back(std::move(..)) and emplace_back(..) may leak if the vector grow fails and an exception is thrown.
ideally, then, this should be push_back(make_unique<>(..))
but we don't have make_unique ;)
- Dominic Hamon
On Dec. 12, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 9:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65180
-----------------------------------------------------------
I depended on this review in r/29083/, but I assumed that the disk transformation would be exposed. The dependency in r/29084/ then assumes that apply() is a const method.
- Ben Mahler
On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2014, 5:57 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
> src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
>
> Diff: https://reviews.apache.org/r/29018/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 29018: Added abstraction for resources
transformation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/
-----------------------------------------------------------
(Updated Dec. 13, 2014, 5:57 a.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030
Repository: mesos-git
Description
-------
Added abstraction for resources transformation. Split from https://reviews.apache.org/r/28720/.
Diffs
-----
include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc
src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92
Diff: https://reviews.apache.org/r/29018/diff/
Testing
-------
make check
Thanks,
Jie Yu