Re: Review Request 65072: Fixed handling of terminal operations in `updateSlave` handler.

2018-01-12 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 65072 was successfully built and tested.

Reviews applied: `['65093', '65095', '65096', '65072']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65072

- Mesos Reviewbot Windows


On Jan. 12, 2018, 10:57 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 12, 2018, 10:57 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal without
> also updating the resources on the agent as any update would already
> be reflected in the new total from the `UpdateSlaveMessage.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in `updateSlave` handler.

2018-01-12 Thread Mesos Reviewbot Windows 

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



FAIL: mesos-java failed to build.

Reviews applied: `['65093', '65095', '65096', '65072']`

Failed command: `cmake.exe --build . --target mesos-java`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65072

Relevant logs:

- 
[mesos-java-build-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65072/logs/mesos-java-build-cmake-stdout.log):

```
  
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3091):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3426):
 warning C4996: 'strerror': This function or variable may be unsafe. Consider 
using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]

Re: Review Request 65072: Fixed handling of terminal operations in `updateSlave` handler.

2018-01-12 Thread Benjamin Bannier 


> On Jan. 11, 2018, 3:07 a.m., Gaston Kleiman wrote:
> > src/master/master.cpp
> > Line 10419 (original), 10447 (patched)
> > 
> >
> > Shouldn't we skip this if the operation was already in a terminal state?

Why would we call `updateOperation` if we wanted no update? I'd prefer to keep 
updating here if asked since I cannot see how it would do harm.


- Benjamin


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


On Jan. 12, 2018, 11:57 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 12, 2018, 11:57 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal without
> also updating the resources on the agent as any update would already
> be reflected in the new total from the `UpdateSlaveMessage.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in `updateSlave` handler.

2018-01-12 Thread Benjamin Bannier 

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

(Updated Jan. 12, 2018, 11:57 a.m.)


Review request for mesos, Gaston Kleiman and Greg Mann.


Bugs: MESOS-8422
https://issues.apache.org/jira/browse/MESOS-8422


Repository: mesos


Description
---

An offer operation can be become terminal between any previously
received non-terminal offer operation status update and receiving an
`UpdateSlaveMessage` (e.g., if the agent failed over, or when the
agent was partitioned from the master).

The master will in its offer operations status handler attempt
to apply operations which became terminal since the last update. At
the same time, the total resources in an `UpdateSlaveMessage` would
already contain the result of applying the operation, and we need to
prevent the master from attempting to apply the same operation twice.

This patch updates the master handler for `UpdateSlaveMessage` to
transition pending operations which are reported as terminal without
also updating the resources on the agent as any update would already
be reflected in the new total from the `UpdateSlaveMessage.


Diffs (updated)
-

  src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab 


Diff: https://reviews.apache.org/r/65072/diff/4/

Changes: https://reviews.apache.org/r/65072/diff/3-4/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-11 Thread Greg Mann 

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


Fix it, then Ship it!





src/master/master.cpp
Lines 7635 (patched)


s/do/does/



src/master/master.cpp
Lines 7659 (patched)


Since we've discovered that updating an operation is not necessarily linked 
to the receipt of an UpdateOperationStatusMessage, I would love if we could 
refactor this in the future so that we don't have to generate an 
UpdateOperationStatusMessage in order to update the operation.

I created a ticket for this work = 
https://issues.apache.org/jira/browse/MESOS-8439
Feel free to link it here if you like.


- Greg Mann


On Jan. 11, 2018, 1:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 11, 2018, 1:53 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-11 Thread Benjamin Bannier 


> On Jan. 10, 2018, 8:18 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 7722 (patched)
> > 
> >
> > s/update/updates/
> > 
> > s/alter/later/

I'd prefer to stay with a singular `update` here.


- Benjamin


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


On Jan. 11, 2018, 2:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 11, 2018, 2:53 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-11 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 65072 was successfully built and tested.

Reviews applied: `['65093', '65095', '65096', '65072']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65072

- Mesos Reviewbot Windows


On Jan. 11, 2018, 2:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 11, 2018, 2:53 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-11 Thread Benjamin Bannier 


> On Jan. 11, 2018, 12:52 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 7722-7723 (patched)
> > 
> >
> > Suggestion:
> > 
> > s/do not trigger another apply of the operation/do not cause us to 
> > apply the operation again/

Went with your second suggestion.


- Benjamin


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


On Jan. 11, 2018, 2:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 11, 2018, 2:53 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-11 Thread Benjamin Bannier 

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

(Updated Jan. 11, 2018, 2:53 p.m.)


Review request for mesos, Gaston Kleiman and Greg Mann.


Changes
---

Addressed comments; properly recovering resources now (in preceeding patches); 
broke up patch into smaller pieces.


Bugs: MESOS-8422
https://issues.apache.org/jira/browse/MESOS-8422


Repository: mesos


Description
---

An offer operation can be become terminal between any previously
received non-terminal offer operation status update and receiving an
`UpdateSlaveMessage` (e.g., if the agent failed over, or when the
agent was partitioned from the master).

The master will in its offer operations status handler attempt
to apply operations which became terminal since the last update. At
the same time, the total resources in an `UpdateSlaveMessage` would
already contain the result of applying the operation, and we need to
prevent the master from attempting to apply the same operation twice.

This patch updates the master handler for `UpdateSlaveMessage` to
transition pending operations which are reported as terminal. For that
we add a new parameter to `updateOperation` to selectively disable
updating of resource state as needed in when called from the
`UpdateSlaveMessage` handler.


Diffs (updated)
-

  src/master/master.cpp 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 


Diff: https://reviews.apache.org/r/65072/diff/2/

Changes: https://reviews.apache.org/r/65072/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-10 Thread Gaston Kleiman 

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




src/master/master.cpp
Line 10419 (original), 10447 (patched)


Shouldn't we skip this if the operation was already in a terminal state?


- Gaston Kleiman


On Jan. 10, 2018, 7:17 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 10, 2018, 7:17 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e6ba53c075174a1e514a395ceb17c26201ec470 
>   src/master/master.cpp 6fc5de89e54ba0b9ae2c4fb475be9878910820d3 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-10 Thread Greg Mann 

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




src/master/master.cpp
Lines 7718 (patched)


s/become/became/



src/master/master.cpp
Lines 7722-7723 (patched)


Suggestion:

s/do not trigger another apply of the operation/do not cause us to apply 
the operation again/



src/master/master.cpp
Lines 7738-7743 (patched)


Generating this message just to update the operation internally doesn't 
seem very elegant, but perhaps it's the best fix given our current 
implementation.



src/master/master.cpp
Lines 7741-7743 (patched)


I think we want to copy these values from `newOperation`?


- Greg Mann


On Jan. 10, 2018, 3:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 10, 2018, 3:17 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e6ba53c075174a1e514a395ceb17c26201ec470 
>   src/master/master.cpp 6fc5de89e54ba0b9ae2c4fb475be9878910820d3 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-10 Thread Greg Mann 

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




src/master/master.cpp
Lines 7722 (patched)


s/update/updates/

s/alter/later/


- Greg Mann


On Jan. 10, 2018, 3:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 10, 2018, 3:17 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e6ba53c075174a1e514a395ceb17c26201ec470 
>   src/master/master.cpp 6fc5de89e54ba0b9ae2c4fb475be9878910820d3 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65072: Fixed handling of terminal operations in updateSlave handler.

2018-01-10 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 65072 was successfully built and tested.

Reviews applied: `['65072']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65072

- Mesos Reviewbot Windows


On Jan. 10, 2018, 7:17 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65072/
> ---
> 
> (Updated Jan. 10, 2018, 7:17 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8422
> https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An offer operation can be become terminal between any previously
> received non-terminal offer operation status update and receiving an
> `UpdateSlaveMessage` (e.g., if the agent failed over, or when the
> agent was partitioned from the master).
> 
> The master will in its offer operations status handler attempt
> to apply operations which became terminal since the last update. At
> the same time, the total resources in an `UpdateSlaveMessage` would
> already contain the result of applying the operation, and we need to
> prevent the master from attempting to apply the same operation twice.
> 
> This patch updates the master handler for `UpdateSlaveMessage` to
> transition pending operations which are reported as terminal. For that
> we add a new parameter to `updateOperation` to selectively disable
> updating of resource state as needed in when called from the
> `UpdateSlaveMessage` handler.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e6ba53c075174a1e514a395ceb17c26201ec470 
>   src/master/master.cpp 6fc5de89e54ba0b9ae2c4fb475be9878910820d3 
> 
> 
> Diff: https://reviews.apache.org/r/65072/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>