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 30694: Fixed bugs in CREATE/DESTROY operation handlers and added tests.
Date Fri, 06 Feb 2015 21:22:29 GMT


> On Feb. 6, 2015, 9:09 p.m., Ben Mahler wrote:
> > src/tests/persistent_volume_tests.cpp, lines 95-127
> > <https://reviews.apache.org/r/30694/diff/2/?file=852444#file852444line95>
> >
> >     camelCase on these? Unless you want these to be structs that are implicitly
convertible to Operation and Resource?

Hum, look like in tests, we follow the google style since we are inherienting from ::testing::Test?
E.g., CreateSlaveFlags, CreateMasterFlags, etc.


> On Feb. 6, 2015, 9:09 p.m., Ben Mahler wrote:
> > src/tests/persistent_volume_tests.cpp, lines 82-92
> > <https://reviews.apache.org/r/30694/diff/2/?file=852444#file852444line82>
> >
> >     Why is this needed? Looks like setting the `--resources` flag with `"role1"`
in the test would be more obvious?

Will have more tests coming and all of them need to set --resources.


> On Feb. 6, 2015, 9:09 p.m., Ben Mahler wrote:
> > src/tests/persistent_volume_tests.cpp, lines 61-80
> > <https://reviews.apache.org/r/30694/diff/2/?file=852444#file852444line61>
> >
> >     Why is this needed?

Framework needs to register using 'role1' so that it can receive offer for 'role1'.


> On Feb. 6, 2015, 9:09 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 279-296
> > <https://reviews.apache.org/r/30694/diff/2/?file=852440#file852440line279>
> >
> >     "checkpointing on the slave", right?
> >     
> >     I think going forward we'll want to pull out our custom filters from the API,
checkpointing seems like an internal filter not relevant to frameworks. Mind adding a TODO
to pull it out?

Done. Added a TODO.


> On Feb. 6, 2015, 9:09 p.m., Ben Mahler wrote:
> > src/tests/persistent_volume_tests.cpp, line 142
> > <https://reviews.apache.org/r/30694/diff/2/?file=852444#file852444line142>
> >
> >     It's not obvious that there's enough disk for the 64MB and 128MB volumes below,
why not just set `--resources` here?

More following tests will use the same structure. Worth pulling it into the test fixture.


- Jie


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


On Feb. 6, 2015, 7:47 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30694/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2015, 7:47 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-2100
>     https://issues.apache.org/jira/browse/MESOS-2100
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixed bugs in CREATE/DESTROY operation handlers and added tests.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 3b57568c10233a0c692787de6464f21af5eaadf4 
>   src/Makefile.am 93537d17d3c7604a8532ee1453e405630c481ddc 
>   src/master/master.hpp dcfd38ae2fa9e1bd0b477e9719724dba37114d30 
>   src/master/master.cpp 234bbecc4205036d790b026abd59100eb188f913 
>   src/tests/persistent_volume_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/30694/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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