You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/01/27 12:54:40 UTC
Review Request 56017: WIP: Added a helper for building a task status
from an existing one.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/
-----------------------------------------------------------
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28
src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd
Diff: https://reviews.apache.org/r/56017/diff/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56017: WIP: Added a helper for building a task
status from an existing one.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review163293
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56016, 56017]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 27, 2017, 12:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 12:54 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28
> src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd
>
> Diff: https://reviews.apache.org/r/56017/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On March 15, 2017, 1:19 p.m., Gast�n Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/56017/diff/5/?file=1665413#file1665413line96>
> >
> > The implementation doesn't create a new task sttus message. It updates the one passed by the user.
> >
> > Given this behaviour, I find the method name and the comment misleading.
>
> Vinod Kone wrote:
> How about we take const & TaskStatus as arg. Then the name makes more sense?
Yes, I like the idea of taking `const TaskStatus&` as arg and creating a copy.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review168999
-----------------------------------------------------------
On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 12:44 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
>
>
> Diff: https://reviews.apache.org/r/56017/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 15, 2017, 1:19 p.m., Gast�n Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/56017/diff/5/?file=1665413#file1665413line96>
> >
> > The implementation doesn't create a new task sttus message. It updates the one passed by the user.
> >
> > Given this behaviour, I find the method name and the comment misleading.
How about we take const & TaskStatus as arg. Then the name makes more sense?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review168999
-----------------------------------------------------------
On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 12:44 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
>
>
> Diff: https://reviews.apache.org/r/56017/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 15, 2017, 1:19 p.m., Gast�n Kleiman wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/56017/diff/5/?file=1665413#file1665413line96>
> >
> > The implementation doesn't create a new task sttus message. It updates the one passed by the user.
> >
> > Given this behaviour, I find the method name and the comment misleading.
>
> Vinod Kone wrote:
> How about we take const & TaskStatus as arg. Then the name makes more sense?
>
> Gast�n Kleiman wrote:
> Yes, I like the idea of taking `const TaskStatus&` as arg and creating a copy.
The implementation does create a new task status message, but from the given one: the compiler will make a copy of the provided task status. The task status passed by user is left intact. Taking `const TaskStatus&` and creating a copy manually is effectively the same, however, less verbose than taking task status by value.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review168999
-----------------------------------------------------------
On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 12:44 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
>
>
> Diff: https://reviews.apache.org/r/56017/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review168999
-----------------------------------------------------------
src/common/protobuf_utils.hpp
Lines 96 (patched)
<https://reviews.apache.org/r/56017/#comment241325>
The implementation doesn't create a new task sttus message. It updates the one passed by the user.
Given this behaviour, I find the method name and the comment misleading.
- Gast�n Kleiman
On March 15, 2017, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 12:44 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
>
>
> Diff: https://reviews.apache.org/r/56017/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/
-----------------------------------------------------------
(Updated March 15, 2017, 12:44 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
Diff: https://reviews.apache.org/r/56017/diff/5/
Changes: https://reviews.apache.org/r/56017/diff/4-5/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/
-----------------------------------------------------------
(Updated March 14, 2017, 2:05 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
Diff: https://reviews.apache.org/r/56017/diff/4/
Changes: https://reviews.apache.org/r/56017/diff/3-4/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/
-----------------------------------------------------------
(Updated March 7, 2017, 8:38 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1
Diff: https://reviews.apache.org/r/56017/diff/3/
Changes: https://reviews.apache.org/r/56017/diff/2-3/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 1, 2017, 7:46 p.m., Vinod Kone wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96-98 (original), 98-100 (patched)
> > <https://reviews.apache.org/r/56017/diff/1-2/?file=1617571#file1617571line98>
> >
> > The last sentence seems un-necssary because the it's clear from the arguments what fields can be overwritten?
I'd rather keep the comment (which should be updated by the way to include `uuid`) in order to make our intent explicit and for example catch situations when this helper and `TaskStatus` go out of sync.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review167556
-----------------------------------------------------------
On Feb. 28, 2017, 3:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0
>
>
> Diff: https://reviews.apache.org/r/56017/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 1, 2017, 7:46 p.m., Vinod Kone wrote:
> > src/common/protobuf_utils.hpp
> > Lines 96-98 (original), 98-100 (patched)
> > <https://reviews.apache.org/r/56017/diff/1-2/?file=1617571#file1617571line98>
> >
> > The last sentence seems un-necssary because the it's clear from the arguments what fields can be overwritten?
>
> Alexander Rukletsov wrote:
> I'd rather keep the comment (which should be updated by the way to include `uuid`) in order to make our intent explicit and for example catch situations when this helper and `TaskStatus` go out of sync.
Up to you. I just find such comments on generic helpers being leaky abstractions (what the default executor's intention is).
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review167556
-----------------------------------------------------------
On Feb. 28, 2017, 3:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0
>
>
> Diff: https://reviews.apache.org/r/56017/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review167556
-----------------------------------------------------------
Fix it, then Ship it!
src/common/protobuf_utils.hpp
Lines 96-98 (original), 98-100 (patched)
<https://reviews.apache.org/r/56017/#comment239433>
The last sentence seems un-necssary because the it's clear from the arguments what fields can be overwritten?
- Vinod Kone
On Feb. 28, 2017, 3:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
> src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0
>
>
> Diff: https://reviews.apache.org/r/56017/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56017: Added a helper for building a task status
from an existing one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 3:48 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Summary (updated)
-----------------
Added a helper for building a task status from an existing one.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340
src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0
Diff: https://reviews.apache.org/r/56017/diff/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 56017: WIP: Added a helper for building a task
status from an existing one.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56017/#review163380
-----------------------------------------------------------
src/common/protobuf_utils.hpp (line 100)
<https://reviews.apache.org/r/56017/#comment235240>
I think `UUID` should also be a required argument instead of auto-magically setting it to random. That way someone doesn't need to look at the comment to know which fields are updated and which fields are not.
```
// Create a new task status from the given task status. Specific fields in `status` can be over-ridden
// in the new status by specifying the appropriate argument.
TaskStatus createTaskStatus(
const TaskStatus& status,
const UUID& uuid,
double timestamp,
...)
```
src/common/protobuf_utils.hpp (line 102)
<https://reviews.apache.org/r/56017/#comment234804>
we don't do const& for POD types.
- Vinod Kone
On Jan. 27, 2017, 12:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56017/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 12:54 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28
> src/common/protobuf_utils.cpp 3ccda1ac2e94ef3fc20d6a2cdb94b6620d914bcd
>
> Diff: https://reviews.apache.org/r/56017/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>