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 69794: Made agent checkpoint operations affecting default resources.
Date Wed, 23 Jan 2019 22:07:46 GMT

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




src/slave/paths.hpp
Lines 377-378 (patched)
<https://reviews.apache.org/r/69794/#comment297898>

    Nit: fits on one line.



src/slave/paths.cpp
Lines 605-606 (patched)
<https://reviews.apache.org/r/69794/#comment297899>

    Nit: fits on one line.



src/slave/paths.cpp
Lines 608 (patched)
<https://reviews.apache.org/r/69794/#comment297901>

    This doesn't seem to match the path noted in the comment above which outlines the agent's
state directories?
    
    I'd be fine with putting the file in the same directory as the others, `resources/`....
`state/` seems a bit generic, since presumably all the directories in that tree contain state
of some type?



src/slave/slave.hpp
Lines 260-261 (original), 260-262 (patched)
<https://reviews.apache.org/r/69794/#comment297902>

    Nit: fits on one line.



src/slave/slave.hpp
Lines 265 (patched)
<https://reviews.apache.org/r/69794/#comment297903>

    We typically use the preceding-underscore convention only for arguments to constructors,
when they initialize data members with the same name. I would recommend using just `resources`
for the argument here.



src/slave/slave.hpp
Lines 847-851 (original), 851-857 (patched)
<https://reviews.apache.org/r/69794/#comment297904>

    I think we can tweak this to make it a bit easier to read at this point, let me know what
you think:
    ```
      // Keeps track of pending operations or terminal operations
      // that have unacknowledged status updates. These operations
      // may affect either agent default resources or resources
      // offered by a resource provider.
    ```



src/slave/slave.cpp
Lines 4396-4397 (original), 4474-4475 (patched)
<https://reviews.apache.org/r/69794/#comment297985>

    Per our offline discussion, we should probably make this call _before_ the operation is
applied so that the checkpointed resources don't reflect the result of the operation. That
way if we crash and re-apply during recovery, we will find that the new checkpointed resources
are different in the body of `syncCheckpointedResources()`.



src/slave/slave.cpp
Lines 4699-4702 (patched)
<https://reviews.apache.org/r/69794/#comment297986>

    Is this necessary? Looks like the call will already be made (unconditionally) in the body
of `removeOperation()`, which is called just before these lines.


- Greg Mann


On Jan. 23, 2019, 6:25 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69794/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2019, 6:25 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Greg Mann.
> 
> 
> Bugs: MESOS-9356
>     https://issues.apache.org/jira/browse/MESOS-9356
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch makes the agent atomically checkpoint resoures and operations
> affecting agent default resources. This is needed in order to provide
> operation feedback for operations affecting agent default resources.
> 
> 
> Diffs
> -----
> 
>   src/slave/paths.hpp 73b94a8df932e40d4f266de01150bb17c2282f22 
>   src/slave/paths.cpp 0ec50d052e29577818e828133a3c501cacf6f2e0 
>   src/slave/slave.hpp 2bcd7a93a8f25b77c71c7f931bfaac87649f987c 
>   src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836 
>   src/tests/slave_tests.cpp 9168e06348a83dcb20400a2fe0e3bb1f26e6ff1b 
> 
> 
> Diff: https://reviews.apache.org/r/69794/diff/2/
> 
> 
> Testing
> -------
> 
> Current tests still pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


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