[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mesos-dev
Subject:    Re: Review Request: Fixed libprocess BufferedRead test.
From:       "Ben Mahler" <benjamin.mahler () gmail ! com>
Date:       2013-01-31 0:51:23
Message-ID: 20130131005123.24967.62946 () reviews ! apache ! org
[Download RAW message or body]


> On Jan. 30, 2013, 8:17 p.m., Benjamin Hindman wrote:
> > 

Let me know if you're going to take another stab at MESOS-326, or if you'd like me to \
as well.


> On Jan. 30, 2013, 8:17 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/io.hpp, lines 19-20
> > <https://reviews.apache.org/r/8697/diff/2/?file=248661#file248661line19>
> > 
> > As we discussed before, libev should fully support files, so this is not a \
> > satisfactory explanation for me. I'd be happy to look more closely into this if \
> > you can tell me how to reproduce it. This goes for the comment in files.cpp too.  \
> >  I agree that we should use stat to detect files versus pipes, sockets, etc and \
> > then use something like libeio, but this should not be necessary.

I agree, and I'm baffled by this as well. You'll find this reproducible on our test \
linux machine. Note that it works perfectly on OSX however.

In my debugging session, I recall there being some observer effects as well.
I'd love for you to take a stab at this as well, if you've got the cycles: \
https://issues.apache.org/jira/browse/MESOS-326


> On Jan. 30, 2013, 8:17 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests.cpp, lines 1377-1378
> > <https://reviews.apache.org/r/8697/diff/2/?file=248662#file248662line1377>
> > 
> > It would be great to use things like EXPECT_FUTURE_WILL_FAIL. If the reason you \
> > didn't use it is because it's missing the 'Seconds(1.0)' component then you \
> > should consider adding a default timeout to the 'await' calls in tests/assert.hpp \
> > (this probably should have been done anyway). Speaking of which, any reason you \
> > went from Seconds(5.0) to Seconds(1.0)? Any reason you need the timeout at all?

1. The timeout is required unfortunately.
2. assert.hpp is only available within mesos for now (until my stout refactorings are \
committed).


> On Jan. 30, 2013, 8:17 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests.cpp, line 1390
> > <https://reviews.apache.org/r/8697/diff/2/?file=248662#file248662line1390>
> > 
> > ASSERT_SOME? Please review this test and use ASSERT_* as necessary.

Ditto: assert.hpp is only within mesos for now.


- Ben


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


On Jan. 16, 2013, 12:55 a.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8697/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2013, 12:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> Commented in MESOS-326.
> 
> 
> This addresses bug MESOS-326.
> https://issues.apache.org/jira/browse/MESOS-326
> 
> 
> Diffs
> -----
> 
> src/files/files.cpp 60b567eb62e84ccc99b0b1978733a0ea96560813 
> third_party/libprocess/include/process/io.hpp \
> 8cf3244e55da95654cea34471ee1eea5e19c872e  third_party/libprocess/src/tests.cpp \
> c0fc2e5af191e9e40d31ed31bd7ad3e852e04583  
> Diff: https://reviews.apache.org/r/8697/diff/
> 
> 
> Testing
> -------
> 
> make check in libprocess
> 
> 
> Thanks,
> 
> Ben Mahler
> 
> 



[prev in list] [next in list] [prev in thread] [next in thread]