You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2019/01/18 23:00:41 UTC
Review Request 69794: Made agent checkpoint operations affecting
default resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/
-----------------------------------------------------------
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 9fd37f5456d45d520d6062577c1692a4be627c0e
src/slave/slave.hpp 2bcd7a93a8f25b77c71c7f931bfaac87649f987c
src/slave/slave.cpp ed92f672f5155d70a36ba3619bb6f06fa09bc836
src/tests/slave_tests.cpp 9168e06348a83dcb20400a2fe0e3bb1f26e6ff1b
Diff: https://reviews.apache.org/r/69794/diff/1/
Testing
-------
Current tests still pass.
Thanks,
Gastón Kleiman
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Jan. 23, 2019, 2:07 p.m., Greg Mann wrote:
> > src/slave/paths.hpp
> > Lines 377-378 (patched)
> > <https://reviews.apache.org/r/69794/diff/1/?file=2120587#file2120587line377>
> >
> > Nit: fits on one line.
I didn't change this one for consistency with most of the rest of the file... but if you really want to join these lines, I can do it =).
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/#review212206
-----------------------------------------------------------
On Jan. 23, 2019, 3:23 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, 3:23 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/3/
>
>
> Testing
> -------
>
> Current tests still pass.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/
-----------------------------------------------------------
(Updated Jan. 28, 2019, 3:25 p.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Rebased.
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 (updated)
-----
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/5/
Changes: https://reviews.apache.org/r/69794/diff/4-5/
Testing
-------
Current tests still pass.
Thanks,
Gastón Kleiman
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/#review212340
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Jan. 24, 2019, 12:16 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69794/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2019, 12:16 a.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/4/
>
>
> Testing
> -------
>
> Current tests still pass.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/
-----------------------------------------------------------
(Updated Jan. 23, 2019, 4:16 p.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Updated proto filename.
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 (updated)
-----
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/4/
Changes: https://reviews.apache.org/r/69794/diff/3-4/
Testing
-------
Current tests still pass.
Thanks,
Gastón Kleiman
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/
-----------------------------------------------------------
(Updated Jan. 23, 2019, 3:23 p.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Addressed feedback.
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/69794/diff/2-3/
Testing
-------
Current tests still pass.
Thanks,
Gastón Kleiman
Re: Review Request 69794: Made agent checkpoint operations affecting
default resources.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69794/
-----------------------------------------------------------
(Updated Jan. 23, 2019, 10:25 a.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Rebaesd.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/69794/diff/1-2/
Testing
-------
Current tests still pass.
Thanks,
Gastón Kleiman