mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 28720: Adjusted the calculation of unused resources in _launchTasks by considering persistent disk acquisition.
Date Thu, 18 Dec 2014 19:20:58 GMT


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1897-1902
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1897>
> >
> >     Ah, we should consider how to make these unit testable, since ResourceValidator
doesn't actually need Framework and Slave.
> >     
> >     Let's punt for now, but this seems like a pretty dangerous area since we're
not going to write big integration tests to catch all the cases. I can think about this.
> >     
> >     Commented on [MESOS-1064](https://issues.apache.org/jira/browse/MESOS-1064)
> >     
> >     One idea was to pull out validators as a series of functions, when we compose
validators we bind in the appropriate arugments. This means not all validators need to take
the same arguments, but we can invoke them through a no-argument function operator, since
they have everything bound in already.
> 
> Michael Park wrote:
>     +1, also added to [MESOS-1064](https://issues.apache.org/jira/browse/MESOS-1064)

Added a comment to MESOS-1064 as well.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1905-1909
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1905>
> >
> >     (1) is enforced via validateDiskInfo, right? Let's add that to the comment.
> >     
> >     The part that I'm a bit confused about is, if the framework changes the role
(e.g. "disk(foo):1<ID1>; disk(bar):1<ID1>") where do we catch that?
> >     
> >     The comment here seems to ignore the fact that the framework could change the
role. Is there something I'm missing that should be documented here?

Adjusted the comments.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1910
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line1910>
> >
> >     Mind adding newlines when we squash comments, NOTEs, and TODOs together?
> >     
> >     ```
> >     // Comment ...
> >     //
> >     // NOTE: ...
> >     //
> >     // TODO(jieyu): ...
> >     ```
> >     
> >     Does this seem a bit easier on the eyes?

Done.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2076
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2076>
> >
> >     s/resource/disk/ ?

Done.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2078
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2078>
> >
> >     This comment looks inaccurate, you're only checking the offered resources here?
> >     
> >     ```
> >     // Ensure the persistence ID for this role does not already exist in the offered
resources.
> >     ```
> >     
> >     Could you add a TODO here for checking the slave's used resources?
> >     
> >     Once we are checking the slave's used resources, (in https://reviews.apache.org/r/28886/),
is it still necessary to look at the offered resources? Won't the task's resources get added
in the slave's used resources after the validation passes?

It's still needed because if not, we may end up having two disks with the same id (different
size). At the time the validation happens, the this task's resource hasn't been added to the
slave's used resources yet.

Added a TODO.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/tests/resource_offers_tests.cpp, line 995
> > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line995>
> >
> >     Mind including a summary comment here?
> >     
> >     ```
> >     // This test ensures that a persistent disk that is larger than
> >     // the offered disk resources results in a failed task.
> >     ```
> >     
> >     Maybe a more precise name?
> >     
> >     AcquirePersistentDiskTooBig?

Done.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/tests/resource_offers_tests.cpp, line 1039
> > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line1039>
> >
> >     Maybe comment on the fact that it's too big? That's why the task fails, right?

Done.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/tests/resource_offers_tests.cpp, lines 1043-1045
> > <https://reviews.apache.org/r/28720/diff/7/?file=795194#file795194line1043>
> >
> >     Is this comment accurate? Where is the non-persistent disk?

Fixed here and everywhere else.


> On Dec. 18, 2014, 2:31 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 2083
> > <https://reviews.apache.org/r/28720/diff/7/?file=795193#file795193line2083>
> >
> >     Would love to have a test for this case! :)

Sure.


- Jie


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


On Dec. 17, 2014, 11:09 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28720/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2014, 11:09 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
>     https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Introduced an abstraction for mutating resources. Acquiring persistent disk is one type
of resources mutation.
> 
> Infer persistent disk acquisitions from resources and check resource usage against adjusted
total resources.
> 
> Adjusted the calculation of unused resources in _launchTasks by considering persistent
disk acquisition.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/tests/resource_offers_tests.cpp e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 
> 
> Diff: https://reviews.apache.org/r/28720/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message