You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/03/22 16:57:51 UTC
Review Request 70282: Added new example framework for operation
feedback.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/
-----------------------------------------------------------
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/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review213924
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['70281', '70282']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2995/mesos-review-70282
Relevant logs:
- [mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2995/mesos-review-70282/logs/mesos-tests.log):
```
W0322 18:41:34.561565 27736 slave.cpp:3932] Ignoring shutdown framework 8c014286-17b2-40d2-bd53-75459bc9a259-0000 because it is terminating
I0322 18:41:34.564551 19516 master.cpp:1295] Agent 8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 (windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0322 18:41:34.564551 19516 master.cpp:3330] Disconnecting agent 8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 (windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 18:41:34.564551 19516 master.cpp:3349] Deactivating agent 8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 (windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 18:41:34.564551 30388 hierarchical.cpp:391] Removed framework 8c014286-17b2-40d2-bd53-75459bc9a259-0000
I0322 18:41:34.565562 30388 hierarchical.cpp:828] Agent 8c014286-17b2-40d2-bd53-75459bc9a259-S0 deactivated
I0322 18:41:34.566567 19516 containerizer.cpp:2576] Destroying container 6fe13f05-451c-4b4e-8d48-041309340bfb in RUNNING state
I0322 18:41:34.566567 19516 containerizer.cpp:3278] Transitioning the state of container 6fe13f05-451c-4b4e-8d48-041309340bfb from RUNNING to DESTROYING
I0322 18:41:34.567555 19516 launcher.cpp:161] Asked to destroy container 6fe13f05-451c-4b4e-8d48-041309340bfb
W0322 18:41:34.568560 30064 process.cpp:1423] Failed to recv on socket WindowsFD::Type::SOCKET=3600 to peer '192.10.1.6:58047': IO failed with error code: The specified network name is no longer available.
W0322 18:41:34.568560 30064 process.cpp:838] Failed to recv on socket WindowsFD::Type::SOCKET=4304 to peer '192.10.1.6:58048': IO failed with error code: The specified network name is no longer available.
I0322 18[ OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (589 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (607 ms total)
[----------] Global test environment tear-down
[==========] 1124 tests from 107 test cases ran. (507237 ms total)
[ PASSED ] 1123 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] DockerFetcherPluginTest.INTERNET_CURL_FetchBlob
1 FAILED TEST
YOU HAVE 231 DISABLED TESTS
:41:34.573561 30388 containerizer.cpp:3117] Container 6fe13f05-451c-4b4e-8d48-041309340bfb has exited
I0322 18:41:34.602557 30680 master.cpp:1135] Master terminating
I0322 18:41:34.603572 12112 hierarchical.cpp:679] Removed agent 8c014286-17b2-40d2-bd53-75459bc9a259-S0
I0322 18:41:35.005559 30064 process.cpp:927] Stopped the socket accept loop
```
- Mesos Reviewbot Windows
On March 22, 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 22, 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/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review213929
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70281, 70282]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 22, 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 22, 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/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/
-----------------------------------------------------------
(Updated March 29, 2019, 1:52 p.m.)
Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
Changes
-------
Handled subscription failures; added unit test.
Repository: mesos
Description
-------
This adds a new example framework showcasing a possible
implementation of the newly added operation feedback API.
Diffs (updated)
-----
src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d
src/examples/CMakeLists.txt f47753980a0bf8d23757df0e7be859db34ae1822
src/examples/operation_feedback_framework.cpp PRE-CREATION
src/tests/CMakeLists.txt ab7f6c2027d937038ee70145705b699c4fb8f05c
src/tests/examples_tests.cpp cadf1d03276a9fdfeedaa2cbab1decfb68ae8a35
src/tests/operation_feedback_framework_test.sh PRE-CREATION
Diff: https://reviews.apache.org/r/70282/diff/4/
Changes: https://reviews.apache.org/r/70282/diff/3-4/
Testing (updated)
-------
Internal CI results:
Platform | Duration | All | Failed | Skipped
-- | -- | -- | -- | --
FLAG=Plain,label=mesos-ec2-ubuntu-16.04 | 18 min | 3325 | 0 | 0
FLAG=BUILD_ISOLATORS,label=mesos-ec2-ubuntu-16.04 | 5.7 sec | 610 | 0 | 0
FLAG=CMake,label=mesos-ec2-centos-7 | 23 min | 3269 | 1 | 0
FLAG=SSL,label=mesos-ec2-centos-6 | 16 min | 3292 | 0 | 0
FLAG=SSL,label=mesos-ec2-debian-9 | 17 min | 3310 | 0 | 0
FLAG=CMake,label=mesos-ec2-ubuntu-16.04 | 20 min | 3278 | 0 | 0
FLAG=Clang,label=mesos-ec2-ubuntu-16.04 | 18 min | 3325 | 0 | 0
FLAG=SSL,label=mesos-ec2-ubuntu-16.04 | 19 min | 3396 | 0 | 0
FLAG=SSL,label=mesos-ec2-centos-7 | 20 min | 3394 | 0 | 0
FLAG=SSL,label=mesos-ec2-debian-8 | 20 min | 3392 | 0 | 0
FLAG=SSL,label=mesos-ec2-ubuntu-14.04 | 19 min | 3395 | 0 | 0
FLAG=SSL,label=mac | 9 min 17 sec | 2788 | 0 | 0
FLAG=Plain,label=mesos-ec2-centos-7 | 20 min | 3316 | 0 | 0
Thanks,
Benno Evers
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review214080
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review214061
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70281, 70282]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 26, 2019, 9:57 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 9:57 a.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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review214109
-----------------------------------------------------------
src/examples/operation_feedback_framework.cpp
Lines 16 (patched)
<https://reviews.apache.org/r/70282/#comment300286>
Ah, sorry, gave a ship-it too soon :) How about adding a shell script for this example framework like we have for the others, in 'src/tests'? This will serve as a unit test that the example framework code is sane, as well as additional testing for the operation feedback feature.
- Greg Mann
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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review214060
-----------------------------------------------------------
PASS: Mesos patch 70282 was successfully built and tested.
Reviews applied: `['70281', '70282']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3018/mesos-review-70282
- Mesos Reviewbot Windows
On March 26, 2019, 9:57 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 9:57 a.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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed review feedback.
Repository: mesos
Description
-------
This adds a new example framework showcasing a possible
implementation of the newly added operation feedback API.
Diffs (updated)
-----
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/
Changes: https://reviews.apache.org/r/70282/diff/2-3/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review213990
-----------------------------------------------------------
PASS: Mesos patch 70282 was successfully built and tested.
Reviews applied: `['70281', '70282']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3008/mesos-review-70282
- Mesos Reviewbot Windows
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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70282/#review214002
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70281, 70282]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Benno Evers <be...@mesosphere.com>.
> 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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 70282: Added new example framework for operation
feedback.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated `TASK_LOST` handling after realizing that this update can still be received by partition-aware frameworks.
Repository: mesos
Description
-------
This adds a new example framework showcasing a possible
implementation of the newly added operation feedback API.
Diffs (updated)
-----
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/
Changes: https://reviews.apache.org/r/70282/diff/1-2/
Testing
-------
Thanks,
Benno Evers