mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Merry <bme...@ska.ac.za>
Subject Re: Review Request 55761: Fixed name matching for automatic resources.
Date Tue, 24 Jan 2017 14:59:38 GMT


> On Jan. 24, 2017, 1:47 a.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.cpp, line 82
> > <https://reviews.apache.org/r/55761/diff/1/?file=1610536#file1610536line82>
> >
> >     s/parts/resources_/

Are you sure you want this? According to the style guide, "Some trailing underscores are used
to distinguish between similar variables in the same scope (think prime symbols), but this
should be avoided as much as possible, including removing existing instances in the code base."

I agree "parts" is not the most descriptive. Maybe "resourceList"? I've changed it to that
for now.


> On Jan. 24, 2017, 1:47 a.m., Vinod Kone wrote:
> > src/tests/containerizer/containerizer_tests.cpp, lines 76-77
> > <https://reviews.apache.org/r/55761/diff/1/?file=1610537#file1610537line76>
> >
> >     also, can you just use "stringify" here?

I don't follow. What do you want me to stringify? (I'm not familiar with which Mesos classes
can be stringified and what the string representation yields).


> On Jan. 24, 2017, 1:47 a.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.cpp, lines 73-74
> > <https://reviews.apache.org/r/55761/diff/1/?file=1610536#file1610536line73>
> >
> >     What is "this list" referring to? I thought the previous comment was clear enough?

It refers to the list defined immediately below, but I've edited the comment to make it a
bit closer to the original.


> On Jan. 24, 2017, 1:47 a.m., Vinod Kone wrote:
> > src/tests/containerizer/containerizer_tests.cpp, line 56
> > <https://reviews.apache.org/r/55761/diff/1/?file=1610537#file1610537line56>
> >
> >     we typicall use camel case in mesos. here you can just do,
> >     
> >     s/expected_ports/ports/

Thanks, I'd been trying to follow the style guide but habits slipped through here.


- Bruce


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


On Jan. 24, 2017, 2:59 p.m., Bruce Merry wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55761/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6821
>     https://issues.apache.org/jira/browse/MESOS-6821
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Instead of doing a simple substring search for known resources like
> "cpus" to decide whether they have been explicitly specified, it now
> parses the resource string to check that a resource with that exact
> name was specified.
> 
> This splits out a fromString static member function from the start of
> Resources::parse that does parsing using either fromJSON or
> fromSimpleString and makes it available to use in
> mesos::internal::slave::Containerizer::resources.
> 
> Fixes MESOS-6821.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   src/Makefile.am 3d0e82cb522d379923e00e5e98e9c9f646fdd3db 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c

>   src/tests/containerizer/containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55761/diff/
> 
> 
> Testing
> -------
> 
> Regression test added.
> 
> 
> Thanks,
> 
> Bruce Merry
> 
>


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