mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 69162: Added agent and resource provider IDs to operation status messages.
Date Tue, 20 Nov 2018 01:57:04 GMT

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




include/mesos/scheduler/scheduler.proto
Line 140 (original), 140 (patched)
<https://reviews.apache.org/r/69162/#comment295424>

    It seems more consistent with `TaskStatus` if we put `slave_id` and `resource_provider_id`
in `OperationStatus`. What's the pros and cons of doing that instead?



include/mesos/scheduler/scheduler.proto
Lines 146 (patched)
<https://reviews.apache.org/r/69162/#comment295416>

    s/ a / an /



include/mesos/scheduler/scheduler.proto
Lines 149 (patched)
<https://reviews.apache.org/r/69162/#comment295422>

    Should we be explicit about what cases they are? If it's not possible, maybe say the following
instead?
    ```
    // In certain cases, e.g., invalid operations, neither `slave_id` nor
    // `resource_provider_id` will be set, and the scheduler does not need to
    // acknowledge this status update.
    ```



include/mesos/v1/scheduler/scheduler.proto
Lines 144 (patched)
<https://reviews.apache.org/r/69162/#comment295420>

    s/ a / an /



include/mesos/v1/scheduler/scheduler.proto
Lines 147 (patched)
<https://reviews.apache.org/r/69162/#comment295423>

    Ditto.



src/messages/messages.cpp
Lines 82-84 (patched)
<https://reviews.apache.org/r/69162/#comment295417>

    Nit: How about following what clang-format gives us here?
    ```
    if (left.has_resource_provider_id() &&
        left.resource_provider_id() != right.resource_provider_id()) {
    ```
    The conditions and the body can be easily distinguished by different indentations above.



src/messages/messages.cpp
Lines 146-149 (original), 156-162 (patched)
<https://reviews.apache.org/r/69162/#comment295418>

    How about the following:
    ```
      if (update.has_resource_provider_id()) {
        stream << " from resource provider " << update.resource_provider_id();
      }
    
      if (update.has_slave_id()) {
        stream << " on agent " << update.slave_id();
      }
    ```


- Chun-Hung Hsiao


On Nov. 12, 2018, 8:49 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2018, 8:49 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
>     https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch add agent and resource provide IDs to
> `UpdateOperationStatus` and `UpdateOperationStatusMessage`. With that
> frameworks are able to reconcile enough information after failover to
> construct operation acknowledgements.
> 
> We will add code to populate these fields in a follow-up patch.
> 
> 
> Diffs
> -----
> 
>   include/mesos/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f 
>   include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c

>   src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d 
>   src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb 
>   src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7 
> 
> 
> Diff: https://reviews.apache.org/r/69162/diff/3/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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