> On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote: > > src/slave/slave.cpp > > Lines 3743 (patched) > > <https://reviews.apache.org/r/63751/diff/4/?file=1893429#file1893429line3743> > > > > For old operations, I think we should apply the conversion (i.e., > > change `totalResources`) in `addOfferOperation` becuase we speculatively > > assume that it'll succeeds. > > > > This makes sense for old operation that has a resource provider. The > > current code is buggy because we don't update agent `totalResources` for > > those old operations. > > > > Then, you don't have to do this assignment here anymore. We still need > > to calculate the checkpointed resources so that we can call the old > > checkpointResources handler. > > > > One unfortunate thing is that `checkpointResources` method will also > > set `totalResources`. This is in fact a bug: > > > > ``` > > // This is a sanity check to verify that the new checkpointed > > // resources are compatible with the agent resources specified > > // through the '--resources' command line flag. The resources > > // should be guaranteed compatible by the master. > > Try<Resources> _totalResources = applyCheckpointedResources( > > info.resources(), > > newCheckpointedResources); > > > > CHECK_SOME(_totalResources) > > << "Failed to apply checkpointed resources " > > << newCheckpointedResources << " to agent's resources " > > << info.resources(); > > > > totalResources = _totalResources.get(); > > ``` > > > > Setting agent resources to `_totalResources.get()` will break the agent > > resource accounting (if resource provider has resources). We might need to > > introduce a parameter to `checkpointResources` to not update > > `totalResources`.
This is addressed in #63798. > On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp > > Lines 94 (patched) > > <https://reviews.apache.org/r/63751/diff/4/?file=1893430#file1893430line94> > > > > Can you add a comment about what that `bool` represents? Ditto in > > ReservationTest Changed it to an enumeration, as that is more clear than a boolean and can be named. - Jan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63751/#review191114 ----------------------------------------------------------- On Nov. 22, 2017, 1:11 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63751/ > ----------------------------------------------------------- > > (Updated Nov. 22, 2017, 1:11 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8211 > https://issues.apache.org/jira/browse/MESOS-8211 > > > Repository: mesos > > > Description > ------- > > Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent > 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'. > The agent will then figure out how to apply the operation. For agent > local resources the agent will checkpoint resources. > > > Diffs > ----- > > src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee > src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc > src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 > src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 > src/tests/persistent_volume_tests.cpp > acfeac16884b00581a3523607ff26f44f6dca53a > src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 > > > Diff: https://reviews.apache.org/r/63751/diff/8/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
