> On Jan. 31, 2017, 5:06 p.m., Guangya Liu wrote: > > src/tests/master_allocator_tests.cpp, lines 1751-1752 > > <https://reviews.apache.org/r/55852/diff/1/?file=1612985#file1612985line1751> > > > > This is only checking one offer but not two? > > Jiang Yan Xu wrote: > `framework2offers` is a `Queue`, calling > [get()](https://github.com/apache/mesos/blob/d0f4cd4d9e98c27fd03eeafcef6e16bb04f51bc8/3rdparty/libprocess/include/process/queue.hpp#L55) > on it repeatedly will pop out the elements. > > Guangya Liu wrote: > Ah, I see, can you please update the comments a bit here to reflect this, > then we can ship it. > > Jiang Yan Xu wrote: > What would you like to see? How about > > ``` > // All offers for framework2 are enqueued by now. > ``` > > ? Or did you mean a comment about `Queue::get()`? > > The latter should probably be a comment in `Queue` and not here right? > > Guangya Liu wrote: > How about > > ``` > // All offers for framework2 are enqueued by now, the > // `Queue::get()` will pop out the elements. > ``` > > BTW: I did not saw any comments in `Queue::get()`, so it would be great > if we can put some comments here, perhaps also need a follow up patch to > update the comments for `Queue::get()`, comments? > > Guangya Liu wrote: > @Yan, just think more, how about update the function name for `Queue` > from `Queue::put` and `Queue::get` to `Queue::push` and `Queue::pop`? My > thinking is `get` should be a `readonly` operation. > > Jiang Yan Xu wrote: > I am reluctant to add comments to explain a particular API. We can > certainly improve docuemntation around `Queue`, would you like to file a JIRA? > > As for there, how about I make the code more obvious? > > ``` > for (int i = 0; i < 2; i++) { > Future<Offer> offer = framework2offers.get(); > > // All offers for framework2 are enqueued by now. > AWAIT_READY(offer); > EXPECT_EQ(Resources(offer->resources()), > Resources::parse(agentResources).get()); > } > ```
re push/pop, it's a separate discussion but my feeling is that `pop()` doesn't convey the sense that it's an async operation but agreed that perhaps there could be a better name than `get()`. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55852/#review163762 ----------------------------------------------------------- On Jan. 31, 2017, 1:03 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55852/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2017, 1:03 a.m.) > > > Review request for mesos, Guangya Liu and Jacob Janco. > > > Bugs: MESOS-6904 > https://issues.apache.org/jira/browse/MESOS-6904 > > > Repository: mesos > > > Description > ------- > > - This test is broken by the batched allocation change. > > > Diffs > ----- > > src/tests/master_allocator_tests.cpp > 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 > > Diff: https://reviews.apache.org/r/55852/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
