mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 70282: Added new example framework for operation feedback.
Date Wed, 27 Mar 2019 02:20:44 GMT


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 61-103 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line61>
> >
> >     Should we just enclose everything outside of `main()` in the `mesos::v1` namespace?
WDYT?

That would look a bit weird to me, tbh, since the framework is not supposed to be part of
the mesos v1 API.

Maybe we should enclose it in a namespace `mesos::examples` instead? Although I think it might
be better to do this for all frameworks at once in a later review, to keep things consistent.


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 142-146 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line142>
> >
> >     I'm not sure I understand this comment; is it suggesting that reservations are
only useful for persistent volumes? That isn't accurate, as we've seen production frameworks
which also use reservations for cpu and memory.

The main point was supposed to be that a real framework would just put `RESERVE` and `LAUNCH`
in the same offer as opposed to splitting it in two stages. On re-reading, I agree that the
comment was a bit confusing and removed the paragraph.


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 185 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line185>
> >
> >     Looks like an invalid resource string would lead to an opaque error from the
`get()` code. How about adding a validation function for the `resources` flag member which
verifies that it will parse successfully, and adding a CHECK_SOME here before calling `get()`?

Yeah, the validation is actually already in place in the very next line, but it's useless
in this version due to the `get()` ;)

It's solved anyways by changing the signature per your other comment.


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 353 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line353>
> >
> >     Shouldn't we also verify here that the offer contains the reserved resources
we wish to unreserve? I think it's possible that we just ran a new task on the reserved resources
from a previous task, in which case there may be no more reserved resources left in the offer?

We can add that as a sanity check, but logically it should never happen: Each task is only
supposed to be launched on the resources that were reserved specifically for that task.


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 443-445 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line443>
> >
> >     This should only be unreachable if we set the PARTITION_AWARE capability in
the framework info; I would recommend that we do set that capability.

It's actually reachable even if we set the `PARTITION_AWARE` capability, see also http://mesos.apache.org/documentation/latest/task-state-reasons/

Fixed by treating this like a regular task failure.


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 574 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line574>
> >
> >     Why is OPERATION_UNKNOWN considered unreachable here but the reserve handler
simply returns? I think you could probably just return in this case as well.

Right, not sure how it ended up down there :D


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 583 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line583>
> >
> >     Is this ever initialized in this file? i.e. we need an initial invocation of
`reconcileOperations()` to begin the delay loop?

Whoops, this used to be in the `connected()`-handler, but it seems like it didn't survive
refactoring :D


> On March 25, 2019, 8:24 p.m., Greg Mann wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 620 (patched)
> > <https://reviews.apache.org/r/70282/diff/1/?file=2133572#file2133572line620>
> >
> >     We should probably verify before this call is sent that the list of operations
in the call is not empty.

We also want to accept when the list of operations is empty, to decline the offer. I'll add
a comment to clarify.


- Benno


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


On March 26, 2019, 4:57 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> -----------------------------------------------------------
> 
> (Updated March 26, 2019, 4:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a new example framework showcasing a possible
> implementation of the newly added operation feedback API.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/examples/CMakeLists.txt f47753980a0bf8d23757df0e7be859db34ae1822 
>   src/examples/operation_feedback_framework.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt ab7f6c2027d937038ee70145705b699c4fb8f05c 
> 
> 
> Diff: https://reviews.apache.org/r/70282/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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