mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <xuj...@apple.com>
Subject Re: Review Request 55852: Fixed MasterAllocatorTest/1.RebalancedForUpdatedWeights.
Date Wed, 01 Feb 2017 02:09:39 GMT


> 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.

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());
}
```


- 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
> 
>


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