mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 70282: Added new example framework for operation feedback.
Date Mon, 25 Mar 2019 20:24:38 GMT

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



Thanks Benno, this looks great!! Some comments below, but the structure of the code is really
nice, good work. Let me know if you have any questions regarding my comments below.


src/examples/operation_feedback_framework.cpp
Lines 61-103 (patched)
<https://reviews.apache.org/r/70282/#comment300143>

    Should we just enclose everything outside of `main()` in the `mesos::v1` namespace? WDYT?



src/examples/operation_feedback_framework.cpp
Lines 136 (patched)
<https://reviews.apache.org/r/70282/#comment300131>

    s/tasks'/task's/



src/examples/operation_feedback_framework.cpp
Lines 139 (patched)
<https://reviews.apache.org/r/70282/#comment300132>

    Nit: period at the end of this sentence.



src/examples/operation_feedback_framework.cpp
Lines 142-146 (patched)
<https://reviews.apache.org/r/70282/#comment300133>

    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.



src/examples/operation_feedback_framework.cpp
Lines 169 (patched)
<https://reviews.apache.org/r/70282/#comment300137>

    Looks like this isn't used.



src/examples/operation_feedback_framework.cpp
Lines 180 (patched)
<https://reviews.apache.org/r/70282/#comment300145>

    What do you think about changing this signature to accept `Resources` and doing the parsing
once in `main()`? This would also allow this function to return `void`.



src/examples/operation_feedback_framework.cpp
Lines 185 (patched)
<https://reviews.apache.org/r/70282/#comment300147>

    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()`?



src/examples/operation_feedback_framework.cpp
Lines 239 (patched)
<https://reviews.apache.org/r/70282/#comment300138>

    Let's also log here that we're sending a SUBSCRIBE call.



src/examples/operation_feedback_framework.cpp
Lines 254 (patched)
<https://reviews.apache.org/r/70282/#comment300148>

    Personally this seems like appropriate for VLOG(1) to me, here and everywhere else in
the file.



src/examples/operation_feedback_framework.cpp
Lines 269 (patched)
<https://reviews.apache.org/r/70282/#comment300149>

    Maybe s/statusUpdate/taskStatusUpdate/ ?



src/examples/operation_feedback_framework.cpp
Lines 335 (patched)
<https://reviews.apache.org/r/70282/#comment300155>

    Isn't it sufficient to just check for `remaining.contains(tx.taskInfo.resources()`?



src/examples/operation_feedback_framework.cpp
Lines 353 (patched)
<https://reviews.apache.org/r/70282/#comment300156>

    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?



src/examples/operation_feedback_framework.cpp
Lines 443-445 (patched)
<https://reviews.apache.org/r/70282/#comment300159>

    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.



src/examples/operation_feedback_framework.cpp
Lines 508 (patched)
<https://reviews.apache.org/r/70282/#comment300161>

    Nit: remove period at the end of log line. Here and elsewhere. (http://mesos.apache.org/documentation/latest/c++-style-guide/#strings)



src/examples/operation_feedback_framework.cpp
Lines 518 (patched)
<https://reviews.apache.org/r/70282/#comment300153>

    Why not set the agent ID when we first know it, at the beginning of the task lifecycle
in `resourceOffers()`? This would make the `CHECK(tx.taskInfo.has_agent_id());` in that function
less concerning at first glance.



src/examples/operation_feedback_framework.cpp
Lines 523-526 (patched)
<https://reviews.apache.org/r/70282/#comment300163>

    This would happen if a new operation state were added in the future and sent to this framework.
I would recommend simply returning here.



src/examples/operation_feedback_framework.cpp
Lines 573 (patched)
<https://reviews.apache.org/r/70282/#comment300164>

    Ditto regarding UNSUPPORTED here; I would recommend returning.



src/examples/operation_feedback_framework.cpp
Lines 574 (patched)
<https://reviews.apache.org/r/70282/#comment300165>

    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.



src/examples/operation_feedback_framework.cpp
Lines 583 (patched)
<https://reviews.apache.org/r/70282/#comment300166>

    Is this ever initialized in this file? i.e. we need an initial invocation of `reconcileOperations()`
to begin the delay loop?



src/examples/operation_feedback_framework.cpp
Lines 604-607 (patched)
<https://reviews.apache.org/r/70282/#comment300167>

    We should also add the agent ID here.



src/examples/operation_feedback_framework.cpp
Lines 620 (patched)
<https://reviews.apache.org/r/70282/#comment300168>

    We should probably verify before this call is sent that the list of operations in the
call is not empty.



src/examples/operation_feedback_framework.cpp
Lines 698-700 (patched)
<https://reviews.apache.org/r/70282/#comment300146>

    Let's move this comment directly above `taskResources`, since that's the member that the
comment refers to.



src/examples/operation_feedback_framework.cpp
Lines 802-805 (patched)
<https://reviews.apache.org/r/70282/#comment300160>

    Let's set PARTITION_AWARE as well?



src/examples/operation_feedback_framework.cpp
Lines 822 (patched)
<https://reviews.apache.org/r/70282/#comment300142>

    This master flag is no longer necessary, I think this can be removed.


- Greg Mann


On March 25, 2019, 1:50 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> -----------------------------------------------------------
> 
> (Updated March 25, 2019, 1:50 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/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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