You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gaston Kleiman <ga...@mesosphere.io> on 2017/12/01 00:27:38 UTC
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Nov. 30, 2017, 4:27 p.m.)
Review request for mesos and Greg Mann.
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
src/tests/CMakeLists.txt 65fca826e8b46ad064096e0f02b771985bef95d0
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/3/
Changes: https://reviews.apache.org/r/64096/diff/2-3/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192413
-----------------------------------------------------------
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 42-44 (patched)
<https://reviews.apache.org/r/64096/#comment270529>
Move this to outside of the class.
- Gaston Kleiman
On Nov. 30, 2017, 4:27 p.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2017, 4:27 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
> src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
> src/tests/CMakeLists.txt 65fca826e8b46ad064096e0f02b771985bef95d0
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/3/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192477
-----------------------------------------------------------
FAIL: Mesos libprocess-tests failed to build
Reviews applied: `['64093', '64094', '64095', '64096']`
Failed command: `cmake.exe --build . --target libprocess-tests --config Debug`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64096
Relevant logs:
- [libprocess-tests-build-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64096/logs/libprocess-tests-build-cmake-stdout.log):
```
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of data (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of data (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of data (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of data (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning C4996: 'GetVersionExW': was declared deprecated (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
"C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj" (default target) (1) ->
(ClCompile target) ->
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C2039: 'realpath': is not a member of 'os' (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\http_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C3861: 'realpath': identifier not found (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\http_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C2039: 'realpath': is not a member of 'os' (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\socket_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C3861: 'realpath': identifier not found (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\socket_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C2039: 'realpath': is not a member of 'os' (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
C:\DCOS\mesos\mesos\3rdparty\libprocess\include\process/ssl/gtest.hpp(355): error C3861: 'realpath': identifier not found (compiling source file C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) [C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
96 Warning(s)
6 Error(s)
Time Elapsed 00:04:24.71
```
- [libprocess-tests-CMakeOutput.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64096/logs/libprocess-tests-CMakeOutput.log):
```
Creating directory "C:\DCOS\mesos\CMakeFiles\CMakeTmp\Debug\".
Creating directory "cmTC_81949.dir\Debug\cmTC_81949.tlog\".
InitializeBuildStatus:
Creating "cmTC_81949.dir\Debug\cmTC_81949.tlog\unsuccessfulbuild" because "AlwaysCreate" was specified.
ClCompile:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.10.25017\bin\HostX64\x64\CL.exe /c /Zi /W3 /WX- /diagnostics:classic /Od /Ob0 /D WIN32 /D _WINDOWS /D COMPILER_SUPPORTS_CXX11 /D "CMAKE_INTDIR=\"Debug\"" /D _MBCS /Gm- /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /Fo"cmTC_81949.dir\Debug\" /Fd"cmTC_81949.dir\Debug\vc141.pdb" /Gd /TP /errorReport:queue C:\DCOS\mesos\CMakeFiles\CMakeTmp\src.cxx
Microsoft (R) C/C++ Optimizing Compiler Version 19.10.25019 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
cl /c /Zi /W3 /WX- /diagnostics:classic /Od /Ob0 /D WIN32 /D _WINDOWS /D COMPILER_SUPPORTS_CXX11 /D "CMAKE_INTDIR=\"Debug\"" /D _MBCS /Gm- /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /Fo"cmTC_81949.dir\Debug\" /Fd"cmTC_81949.dir\Debug\vc141.pdb" /Gd /TP /errorReport:queue C:\DCOS\mesos\CMakeFiles\CMakeTmp\src.cxx
src.cxx
Link:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.10.25017\bin\HostX64\x64\link.exe /ERRORREPORT:QUEUE /OUT:"C:\DCOS\mesos\CMakeFiles\CMakeTmp\Debug\cmTC_81949.exe" /INCREMENTAL /NOLOGO kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG /PDB:"C:/DCOS/mesos/CMakeFiles/CMakeTmp/Debug/cmTC_81949.pdb" /SUBSYSTEM:CONSOLE /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"C:/DCOS/mesos/CMakeFiles/CMakeTmp/Debug/cmTC_81949.lib" /MACHINE:X64 /machine:x64 cmTC_81949.dir\Debug\src.obj
cmTC_81949.vcxproj -> C:\DCOS\mesos\CMakeFiles\CMakeTmp\Debug\cmTC_81949.exe
cmTC_81949.vcxproj -> C:/DCOS/mesos/CMakeFiles/CMakeTmp/Debug/cmTC_81949.pdb (Full PDB)
FinalizeBuildStatus:
Deleting file "cmTC_81949.dir\Debug\cmTC_81949.tlog\unsuccessfulbuild".
Touching "cmTC_81949.dir\Debug\cmTC_81949.tlog\cmTC_81949.lastbuildstate".
Done Building Project "C:\DCOS\mesos\CMakeFiles\CMakeTmp\cmTC_81949.vcxproj" (default targets).
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:00:01.80
Source file was:
int main() { return 0; }
```
- [libprocess-tests-CMakeError.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64096/logs/libprocess-tests-CMakeError.log):
```
PrepareForBuild:
Creating directory "cmTC_e3510.dir\Debug\".
Creating directory "C:\DCOS\mesos\CMakeFiles\CMakeTmp\Debug\".
Creating directory "cmTC_e3510.dir\Debug\cmTC_e3510.tlog\".
InitializeBuildStatus:
Creating "cmTC_e3510.dir\Debug\cmTC_e3510.tlog\unsuccessfulbuild" because "AlwaysCreate" was specified.
ClCompile:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.10.25017\bin\HostX64\x64\CL.exe /c /Zi /W3 /WX- /diagnostics:classic /MP /Od /Ob0 /D WIN32 /D _WINDOWS /D UNICODE /D _UNICODE /D "CMAKE_INTDIR=\"Debug\"" /D _UNICODE /D UNICODE /Gm- /RTC1 /MTd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"cmTC_e3510.dir\Debug\" /Fd"cmTC_e3510.dir\Debug\vc141.pdb" /Gd /TC /errorReport:queue C:\DCOS\mesos\CMakeFiles\CMakeTmp\CheckIncludeFile.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.10.25019 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
cl /c /Zi /W3 /WX- /diagnostics:classic /MP /Od /Ob0 /D WIN32 /D _WINDOWS /D UNICODE /D _UNICODE /D "CMAKE_INTDIR=\"Debug\"" /D _UNICODE /D UNICODE /Gm- /RTC1 /MTd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"cmTC_e3510.dir\Debug\" /Fd"cmTC_e3510.dir\Debug\vc141.pdb" /Gd /TC /errorReport:queue C:\DCOS\mesos\CMakeFiles\CMakeTmp\CheckIncludeFile.c
CheckIncludeFile.c
C:\DCOS\mesos\CMakeFiles\CMakeTmp\CheckIncludeFile.c(1): fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory [C:\DCOS\mesos\CMakeFiles\CMakeTmp\cmTC_e3510.vcxproj]
Done Building Project "C:\DCOS\mesos\CMakeFiles\CMakeTmp\cmTC_e3510.vcxproj" (default targets) -- FAILED.
Build FAILED.
"C:\DCOS\mesos\CMakeFiles\CMakeTmp\cmTC_e3510.vcxproj" (default target) (1) ->
(ClCompile target) ->
C:\DCOS\mesos\CMakeFiles\CMakeTmp\CheckIncludeFile.c(1): fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory [C:\DCOS\mesos\CMakeFiles\CMakeTmp\cmTC_e3510.vcxproj]
0 Warning(s)
1 Error(s)
Time Elapsed 00:00:01.15
```
- Mesos Reviewbot Windows
On Dec. 1, 2017, 12:27 a.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 12:27 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
> src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
> src/tests/CMakeLists.txt 65fca826e8b46ad064096e0f02b771985bef95d0
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/3/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192381
-----------------------------------------------------------
src/CMakeLists.txt
Lines 487 (patched)
<https://reviews.apache.org/r/64096/#comment270487>
Perhaps the filenames 'offer_operation.{cpp,hpp}' would suffice, since they sit within the 'status_update_manager' directory. WDYT?
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 17-18 (patched)
<https://reviews.apache.org/r/64096/#comment270499>
s/PROCESS_//
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 46-47 (patched)
<https://reviews.apache.org/r/64096/#comment270505>
Let's be a little more explicit here:
"called in order to forward a new status update to its recipient."
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 79 (patched)
<https://reviews.apache.org/r/64096/#comment270688>
s/exist/exist or is empty/
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 80-83 (patched)
<https://reviews.apache.org/r/64096/#comment270689>
Need to update this comment and the signature for the `strict` param.
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 91-97 (patched)
<https://reviews.apache.org/r/64096/#comment270691>
Can be removed.
src/status_update_manager/offer_operation_status_update_manager.hpp
Lines 102 (patched)
<https://reviews.apache.org/r/64096/#comment270716>
Let's use an `Owned` here instead of a raw pointer.
src/status_update_manager/offer_operation_status_update_manager.cpp
Lines 36-38 (patched)
<https://reviews.apache.org/r/64096/#comment270722>
I think it's a bit more consistent with the spirit of our style guide (i.e. to reduce "jaggedness") to wrap after the '<' and indent 4 spaces:
```
process = new StatusUpdateManagerProcess<
UUID,
OfferOperationStatusUpdateRecord,
OfferOperationStatusUpdate>();
```
http://mesos.apache.org/documentation/latest/c++-style-guide/#function-definition-invocation
Could you switch to that format throughout this chain?
- Greg Mann
On Dec. 1, 2017, 12:27 a.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 12:27 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
> src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
> src/tests/CMakeLists.txt 65fca826e8b46ad064096e0f02b771985bef95d0
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/3/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review193214
-----------------------------------------------------------
Some nits below. I can fix these while committing.
src/status_update_manager/offer_operation.hpp
Lines 17-18 (patched)
<https://reviews.apache.org/r/64096/#comment271770>
Nit: s/__HPP/_HPP/
I'll fix while committing.
src/status_update_manager/offer_operation.hpp
Lines 53 (patched)
<https://reviews.apache.org/r/64096/#comment271772>
Nit: let's put a `that` here. Will add while committing.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 85 (patched)
<https://reviews.apache.org/r/64096/#comment271774>
I think it's more appropriate to leave the `settle()` in the test body, so that it's clear while viewing the tests that this is performed. I can do this while committing.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 90-93 (patched)
<https://reviews.apache.org/r/64096/#comment271775>
Indentation.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 141 (patched)
<https://reviews.apache.org/r/64096/#comment271779>
Why a reference?
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 295 (patched)
<https://reviews.apache.org/r/64096/#comment271776>
s/forwardeda/forwarded./
- Greg Mann
On Dec. 7, 2017, 1:22 a.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 1:22 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 35a602d2afb3a1e6ef76a0b0df2628ce5493dc81
> src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
> src/status_update_manager/offer_operation.hpp PRE-CREATION
> src/status_update_manager/offer_operation.cpp PRE-CREATION
> src/tests/CMakeLists.txt 92db731a81303f0d1d95dfe0b80a0a512e165445
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/10/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Dec. 6, 2017, 5:22 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Addressed feedback.
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 35a602d2afb3a1e6ef76a0b0df2628ce5493dc81
src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
src/status_update_manager/offer_operation.hpp PRE-CREATION
src/status_update_manager/offer_operation.cpp PRE-CREATION
src/tests/CMakeLists.txt 92db731a81303f0d1d95dfe0b80a0a512e165445
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/10/
Changes: https://reviews.apache.org/r/64096/diff/9-10/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
> On Dec. 6, 2017, 12:29 p.m., Greg Mann wrote:
> > src/status_update_manager/offer_operation.hpp
> > Lines 48 (patched)
> > <https://reviews.apache.org/r/64096/diff/9/?file=1909289#file1909289line48>
> >
> > This doesn't need to be `virtual` since we don't expect to use this as a base class. Instead, let's do:
> >
> > ```
> > ~OfferOperationStatusUpdateManager() override;
> > ```
> >
> > `override` will ensure that we get a compile-time error if this method fails to override a base class destructor in the future.
I removed the `virtual` modifier. This is a base clase, so `override` doesn't make sense... should we make the class `final`?
> On Dec. 6, 2017, 12:29 p.m., Greg Mann wrote:
> > src/tests/offer_operation_status_update_manager_tests.cpp
> > Lines 778-780 (patched)
> > <https://reviews.apache.org/r/64096/diff/9/?file=1909292#file1909292line778>
> >
> > Is this record "corrupted"? Looks to me like it's a duplicate?
The file should contain only `OfferOperationStatusUpdateRecord` messages.
The test writes a `OfferOperationStatusUpdate` to the end of the file, so that the recovery process encounters an error.
Added a comment describing this.
- Gaston
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192999
-----------------------------------------------------------
On Dec. 6, 2017, 5:22 p.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2017, 5:22 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 35a602d2afb3a1e6ef76a0b0df2628ce5493dc81
> src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
> src/status_update_manager/offer_operation.hpp PRE-CREATION
> src/status_update_manager/offer_operation.cpp PRE-CREATION
> src/tests/CMakeLists.txt 92db731a81303f0d1d95dfe0b80a0a512e165445
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/10/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192999
-----------------------------------------------------------
src/status_update_manager/offer_operation.hpp
Lines 17 (patched)
<https://reviews.apache.org/r/64096/#comment271452>
Let's include the directory in this (see the headers in 'resource_provider/', for example:
```
#ifndef __STATUS_UPDATE_MANAGER_OFFER_OPERATION__HPP__
```
At the end of the file as well
src/status_update_manager/offer_operation.hpp
Lines 39-42 (patched)
<https://reviews.apache.org/r/64096/#comment271466>
Looks like we usually indent 4 spaces in these cases? Please confirm and update if appropriate, here and elsewhere.
src/status_update_manager/offer_operation.hpp
Lines 47-48 (patched)
<https://reviews.apache.org/r/64096/#comment271469>
To ensure we don't have duplicate status update managers attempting to access the same streams, we could also delete the copy constructor and assignment operator.
src/status_update_manager/offer_operation.hpp
Lines 48 (patched)
<https://reviews.apache.org/r/64096/#comment271467>
This doesn't need to be `virtual` since we don't expect to use this as a base class. Instead, let's do:
```
~OfferOperationStatusUpdateManager() override;
```
`override` will ensure that we get a compile-time error if this method fails to override a base class destructor in the future.
src/status_update_manager/offer_operation.hpp
Lines 60 (patched)
<https://reviews.apache.org/r/64096/#comment271468>
s/@return Whether/Returns whether/
src/status_update_manager/offer_operation.hpp
Lines 97-99 (patched)
<https://reviews.apache.org/r/64096/#comment271470>
We don't usually indent the lines after the NOTE.
src/status_update_manager/offer_operation.cpp
Lines 57-64 (patched)
<https://reviews.apache.org/r/64096/#comment271471>
Indentation. Please fix here and elsewhere.
src/status_update_manager/offer_operation.cpp
Lines 72 (patched)
<https://reviews.apache.org/r/64096/#comment271479>
We should really only do something like this in test code (a `.get()` without a CHECK or `if` guard). Adding the CHECK will get us a stack trace if things go wrong. Instead, here let's do:
```
Try<UUID> operationUuid = UUID::fromBytes(update.operation_uuid());
CHECK_SOME(operationUuid);
return dispatch(
process.get(),
&StatusUpdateManagerProcess<
UUID,
OfferOperationStatusUpdateRecord,
OfferOperationStatusUpdate>::update,
update,
operationUuid.get(),
checkpoint);
```
src/status_update_manager/offer_operation.cpp
Lines 76-79 (patched)
<https://reviews.apache.org/r/64096/#comment271473>
Could use a typedef to clean this up a bit - I'll leave it up to you.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 52 (patched)
<https://reviews.apache.org/r/64096/#comment271496>
Could you add a comment here explaining the motivation for this class?
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 56-58 (patched)
<https://reviews.apache.org/r/64096/#comment271474>
One more newline here.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 62-63 (patched)
<https://reviews.apache.org/r/64096/#comment271475>
const ref?
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/64096/#comment271501>
s/statusUpdatesProcessor/statusUpdateProcessor/
seems more consistent?
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 108 (patched)
<https://reviews.apache.org/r/64096/#comment271505>
Let's s/SUM/status update manager/ throughout this file.
I appreciate the desire for brevity, but I would prefer to make the comments maximally comprehensible to a reader who drops in to look at just a single test in the future, without lots of context.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 108-109 (patched)
<https://reviews.apache.org/r/64096/#comment271507>
The "until" in this comment seems to imply that we are testing retries as well.
Perhaps:
"This test verifies that the status update manager will not retry an update after it has been acknowledged."
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 145-146 (patched)
<https://reviews.apache.org/r/64096/#comment271510>
Ditto on the wording of this comment.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 166 (patched)
<https://reviews.apache.org/r/64096/#comment271513>
Is there a reason not to pause the clock at the beginning of every test case? Perhaps that could even go into the fixture?
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 178 (patched)
<https://reviews.apache.org/r/64096/#comment271517>
If you decide to pause for the entirety of each test, perhaps this final `resume` could go into the fixture as well.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 244 (patched)
<https://reviews.apache.org/r/64096/#comment271520>
Let's declare a `FrameworkID frameworkId` above and pass it here. We can also use it in the `cleanup` call below.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 297-303 (patched)
<https://reviews.apache.org/r/64096/#comment271523>
Is the purpose of this cleanup call to clear the status update manager state? If so, let's say that explicitly in the comment.
It would be more compelling to actually instantiate a new status update manager and then recover from the previous state, but if that's difficult this works as well.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 346 (patched)
<https://reviews.apache.org/r/64096/#comment271525>
Since the framework ID isn't strictly necessary here, it might be nice to omit it and have another way to clear the manager state.
Perhaps the fixture could have a `resetStatusUpdateManager()` method which resets the SUM member?
Here and elsewhere.
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 369 (patched)
<https://reviews.apache.org/r/64096/#comment271526>
s/returns//
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 408-410 (patched)
<https://reviews.apache.org/r/64096/#comment271527>
s/but before any data/but the status update manager crashed before any data/
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 428 (patched)
<https://reviews.apache.org/r/64096/#comment271528>
s/returns//
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 670-671 (patched)
<https://reviews.apache.org/r/64096/#comment271531>
s/ignored/acknowledged/
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 723-724 (patched)
<https://reviews.apache.org/r/64096/#comment271533>
s/ignored/acknowledged/
src/tests/offer_operation_status_update_manager_tests.cpp
Lines 778-780 (patched)
<https://reviews.apache.org/r/64096/#comment271534>
Is this record "corrupted"? Looks to me like it's a duplicate?
- Greg Mann
On Dec. 4, 2017, 11:08 p.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2017, 11:08 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a76ba1e73a3480baba2661b2e680814204df4d3e
> src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449
> src/status_update_manager/offer_operation.hpp PRE-CREATION
> src/status_update_manager/offer_operation.cpp PRE-CREATION
> src/tests/CMakeLists.txt b74fbb9bb19f9d73569b2e83e34d8959ad3aabd4
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/9/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Dec. 4, 2017, 3:08 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Cleaned up the tests.
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88
src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b
src/status_update_manager/offer_operation.hpp PRE-CREATION
src/status_update_manager/offer_operation.cpp PRE-CREATION
src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/7/
Changes: https://reviews.apache.org/r/64096/diff/6-7/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Dec. 4, 2017, 10:56 a.m.)
Review request for mesos and Greg Mann.
Changes
-------
Renamed files, fixed tests.
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88
src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b
src/status_update_manager/offer_operation.hpp PRE-CREATION
src/status_update_manager/offer_operation.cpp PRE-CREATION
src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/6/
Changes: https://reviews.apache.org/r/64096/diff/5-6/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Dec. 1, 2017, 6:46 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Addressed some more comments.
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88
src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b
src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/5/
Changes: https://reviews.apache.org/r/64096/diff/4-5/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/
-----------------------------------------------------------
(Updated Dec. 1, 2017, 6:15 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Addressed **some** of the comments =).
Bugs: MESOS-8197
https://issues.apache.org/jira/browse/MESOS-8197
Repository: mesos
Description
-------
This class will handle the offer operation status updates generated by
the agent and by resource providers.
Diffs (updated)
-----
src/CMakeLists.txt 592489df070a0c4fdee814a4a7b613e62a544f88
src/Makefile.am 08d29ab58fe5c7f5e0c9173b504787ec254ed99b
src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
src/tests/CMakeLists.txt 5e5202079c9798b91028e845490b747d4d1d0a66
src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/64096/diff/4/
Changes: https://reviews.apache.org/r/64096/diff/3-4/
Testing
-------
This patch addes new tests that passed 5000 times on GNU/Linux.
Thanks,
Gaston Kleiman
Re: Review Request 64096: Implemented the
`OfferOperationStatusUpdateManager`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192525
-----------------------------------------------------------
src/status_update_manager/offer_operation_status_update_manager.cpp
Lines 57-59 (patched)
<https://reviews.apache.org/r/64096/#comment270723>
Yesterday we discussed moving the header's `typedef` out to file scope, which would let you use it here.
- Greg Mann
On Dec. 1, 2017, 12:27 a.m., Gaston Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 12:27 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-8197
> https://issues.apache.org/jira/browse/MESOS-8197
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/status_update_manager/offer_operation_status_update_manager.hpp PRE-CREATION
> src/status_update_manager/offer_operation_status_update_manager.cpp PRE-CREATION
> src/tests/CMakeLists.txt 65fca826e8b46ad064096e0f02b771985bef95d0
> src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64096/diff/3/
>
>
> Testing
> -------
>
> This patch addes new tests that passed 5000 times on GNU/Linux.
>
>
> Thanks,
>
> Gaston Kleiman
>
>