Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.

2017-12-13 Thread Jie Yu 

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

(Updated Dec. 13, 2017, 7:34 p.m.)


Review request for mesos, Benjamin Bannier and Jan Schlicht.


Changes
---

addressed comments.


Repository: mesos


Description
---

Given that now we use `UpdateSlaveMessage` to send resource provider
information directly, having resource categories in the message is
unnecessary and misleading.

Instead, this patch introduced a single optional boolean to indicate if
oversubscribed resources need to be updated or not.


Diffs (updated)
-

  src/master/master.cpp 806fbc275af11bb37c2daa369f69a71bc4f36312 
  src/messages/messages.proto e680cd5e4d5a93c3c77309f327844f55fbb239a1 
  src/slave/slave.hpp 7c40fc71b49057fea0cfd85290931fbd0f6a9d62 
  src/slave/slave.cpp d997b4272578efffed05d38771f17df387ccac48 
  src/tests/oversubscription_tests.cpp 3f57ce105e24e9f9cd681d8d984dbe242aa51f75 


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

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.

2017-12-13 Thread Jie Yu 


> On Dec. 13, 2017, 12:20 p.m., Benjamin Bannier wrote:
> > src/slave/slave.cpp
> > Line 6992 (original)
> > 
> >
> > One motivation for such helper functions can be that it becomes clearer 
> > than `UpdateSlaveMessage` should not be constructed manually due to 
> > possible complexity. By removing one of them this is not as apparent 
> > anymore.
> > 
> > I'd suggest to keep this helper and revert changes at the callers so an 
> > up-to-date `oversubscribedResources` is found.

I changed that because merging `update_oversubscribed_resources` field with one 
set to true and one set to false is weird. There is actually a bug previously 
that in generateResourceProviderMessage, we don't set 
`update_oversubscribed_resources`, with leads to an update to the 
oversubscribed resources when people are using resource provider.

I'll stick to the existing code. I'll think about a cleanup to generate those 
messages without looking into member fields (instead, making them a pure 
helper).


- Jie


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


On Dec. 13, 2017, 12:56 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64561/
> ---
> 
> (Updated Dec. 13, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Given that now we use `UpdateSlaveMessage` to send resource provider
> information directly, having resource categories in the message is
> unnecessary and misleading.
> 
> Instead, this patch introduced a single optional boolean to indicate if
> oversubscribed resources need to be updated or not.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 232cc3758f240db626c4fdaf852163fa48af4dd7 
>   src/master/master.cpp efe8b8f1704b314e6e6a4d5632718cab2854e38f 
>   src/messages/messages.proto e680cd5e4d5a93c3c77309f327844f55fbb239a1 
>   src/slave/slave.hpp 7c40fc71b49057fea0cfd85290931fbd0f6a9d62 
>   src/slave/slave.cpp 5869e73ca1c14c99e580da9d7375181da2073ec5 
>   src/tests/oversubscription_tests.cpp 
> 3f57ce105e24e9f9cd681d8d984dbe242aa51f75 
> 
> 
> Diff: https://reviews.apache.org/r/64561/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.

2017-12-13 Thread Jie Yu 


> On Dec. 13, 2017, 12:20 p.m., Benjamin Bannier wrote:
> > src/master/master.hpp
> > Line 502 (original), 502 (patched)
> > 
> >
> > Let's not do this in this patch as it is an unrelated change. Instead 
> > we should clean up all handler at once.

There is a bug in fact because we should consistently use mutated `message_` in 
the following code. Anyway, I'll fix that in a subsequent patch to use rvalue 
references.


- Jie


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


On Dec. 13, 2017, 12:56 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64561/
> ---
> 
> (Updated Dec. 13, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Given that now we use `UpdateSlaveMessage` to send resource provider
> information directly, having resource categories in the message is
> unnecessary and misleading.
> 
> Instead, this patch introduced a single optional boolean to indicate if
> oversubscribed resources need to be updated or not.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 232cc3758f240db626c4fdaf852163fa48af4dd7 
>   src/master/master.cpp efe8b8f1704b314e6e6a4d5632718cab2854e38f 
>   src/messages/messages.proto e680cd5e4d5a93c3c77309f327844f55fbb239a1 
>   src/slave/slave.hpp 7c40fc71b49057fea0cfd85290931fbd0f6a9d62 
>   src/slave/slave.cpp 5869e73ca1c14c99e580da9d7375181da2073ec5 
>   src/tests/oversubscription_tests.cpp 
> 3f57ce105e24e9f9cd681d8d984dbe242aa51f75 
> 
> 
> Diff: https://reviews.apache.org/r/64561/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.

2017-12-13 Thread Benjamin Bannier 

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


Fix it, then Ship it!




This implements the same functionality as https://reviews.apache.org/r/64445/. 
Let's just update this one and drop the other.


src/master/master.hpp
Line 502 (original), 502 (patched)


Let's not do this in this patch as it is an unrelated change. Instead we 
should clean up all handler at once.



src/master/master.cpp
Line 7221 (original), 7221 (patched)


Ditto.



src/master/master.cpp
Line 7249 (original), 7249 (patched)


If the agent did not specify *the* 'update_overs...



src/messages/messages.proto
Line 734 (original), 734 (patched)


It looks like we introduced resource categories in `a88469bfe8` which is 
currently still only contained in `master` and was not part of a release.

We can recycle field number 5.



src/slave/slave.hpp
Line 561 (original), 561 (patched)


Not yours, but could you `s/resoruce/resource/`?



src/slave/slave.cpp
Line 6992 (original)


One motivation for such helper functions can be that it becomes clearer 
than `UpdateSlaveMessage` should not be constructed manually due to possible 
complexity. By removing one of them this is not as apparent anymore.

I'd suggest to keep this helper and revert changes at the callers so an 
up-to-date `oversubscribedResources` is found.


- Benjamin Bannier


On Dec. 13, 2017, 1:56 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64561/
> ---
> 
> (Updated Dec. 13, 2017, 1:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Given that now we use `UpdateSlaveMessage` to send resource provider
> information directly, having resource categories in the message is
> unnecessary and misleading.
> 
> Instead, this patch introduced a single optional boolean to indicate if
> oversubscribed resources need to be updated or not.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 232cc3758f240db626c4fdaf852163fa48af4dd7 
>   src/master/master.cpp efe8b8f1704b314e6e6a4d5632718cab2854e38f 
>   src/messages/messages.proto e680cd5e4d5a93c3c77309f327844f55fbb239a1 
>   src/slave/slave.hpp 7c40fc71b49057fea0cfd85290931fbd0f6a9d62 
>   src/slave/slave.cpp 5869e73ca1c14c99e580da9d7375181da2073ec5 
>   src/tests/oversubscription_tests.cpp 
> 3f57ce105e24e9f9cd681d8d984dbe242aa51f75 
> 
> 
> Diff: https://reviews.apache.org/r/64561/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.

2017-12-12 Thread Mesos Reviewbot Windows 

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



FAIL: Some Mesos tests failed.

Reviews applied: `['64557', '64561']`

Failed command: `D:\DCOS\mesos\src\mesos-tests.exe --verbose`

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

Relevant logs:

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

```

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (2430 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (2455 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (2373 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (2397 ms total)

[--] Global test environment tear-down
[==] 831 tests from 84 test cases ran. (323528 ms total)
[  PASSED  ] 821 tests.
[  FAILED  ] 10 tests, listed below:
[  FAILED  ] OfferOperationStatusUpdateManagerTest.UpdateAndAckNonTerminalUpdate
[  FAILED  ] OfferOperationStatusUpdateManagerTest.RecoverCheckpointedStream
[  FAILED  ] OfferOperationStatusUpdateManagerTest.RecoverEmptyFile
[  FAILED  ] OfferOperationStatusUpdateManagerTest.RecoverTerminatedStream
[  FAILED  ] OfferOperationStatusUpdateManagerTest.IgnoreDuplicateUpdate
[  FAILED  ] 
OfferOperationStatusUpdateManagerTest.IgnoreDuplicateUpdateAfterRecover
[  FAILED  ] OfferOperationStatusUpdateManagerTest.RejectDuplicateAck
[  FAILED  ] 
OfferOperationStatusUpdateManagerTest.RejectDuplicateAckAfterRecover
[  FAILED  ] 
OfferOperationStatusUpdateManagerTest.NonStrictRecoveryCorruptedFile
[  FAILED  ] SlaveTest.ResourceProviderPublishAll

10 FAILED TESTS
  YOU HAVE 201 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64561/logs/mesos-tests-stderr.log):

```
I1213 04:22:15.114867  3156 master.cpp:10154] Updating the state of task 
ad428d78-1454-409e-972f-2ae2f2f736e9 of framework 
c7b0dfca-1739-4662-8320-06724dd392ec- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I1213 04:22:15.115867  5100 slave.cpp:3400] Shutting down framework 
c7b0dfca-1739-4662-8320-06724dd392ec-
I1213 04:22:15.115867  5100 slave.cpp:6114] SI1213 04:22:14.415889  5280 
exec.cpp:162] Version: 1.5.0
I1213 04:22:14.439934  1604 exec.cpp:237] Executor registered on agent 
c7b0dfca-1739-4662-8320-06724dd392ec-S0
I1213 04:22:14.442921  6824 executor.cpp:171] Received SUBSCRIBED event
I1213 04:22:14.446918  6824 executor.cpp:175] Subscribed executor on 
build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I1213 04:22:14.447918  6824 executor.cpp:171] Received LAUNCH event
I1213 04:22:14.451931  6824 executor.cpp:637] Starting task 
ad428d78-1454-409e-972f-2ae2f2f736e9
I1213 04:22:14.533891  6824 executor.cpp:477] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I1213 04:22:15.088878  6824 executor.cpp:650] Forked command at 3060
I1213 04:22:15.117868  2428 exec.cpp:435] Executor asked to shutdown
I1213 04:22:15.117868  6824 executor.cpp:171] Received SHUTDOWN event
I1213 04:22:15.117868  6824 executor.cpp:747] Shutting down
I1213 04:22:15.117868  6824 executor.cpp:854] Sending SIGTERM to process tree 
at pid 3hutting down executor 'ad428d78-1454-409e-972f-2ae2f2f736e9' of 
framework c7b0dfca-1739-4662-8320-06724dd392ec- at 
executor(1)@10.3.1.11:61392
I1213 04:22:15.116868  5100 slave.cpp:909] Agent terminating
W1213 04:22:15.116868  5100 slave.cpp:3396] Ignoring shutdown framework 
c7b0dfca-1739-4662-8320-06724dd392ec- because it is terminating
I1213 04:22:15.117868  3156 master.cpp:10260] Removing task 
ad428d78-1454-409e-972f-2ae2f2f736e9 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework c7b0dfca-1739-4662-8320-06724dd392ec- on 
agent c7b0dfca-1739-4662-8320-06724dd392ec-S0 at slave(328)@10.3.1.11:61371 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I1213 04:22:15.119868  3156 master.cpp:1305] Agent 
c7b0dfca-1739-4662-8320-06724dd392ec-S0 at slave(328)@10.3.1.11:61371 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I1213 04:22:15.119868  3156 master.cpp:3364] Disconnecting agent 
c7b0dfca-1739-4662-8320-06724dd392ec-S0 at slave(328)@10.3.1.11:61371 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I1213 04:22:15.119868  9832 containerizer.cpp:2328] Destroying container 
34f12d3c-f5e9-496b-be50-1f57768b3e80 in RUNNING state
I1213 04:22:15.120872  6840