mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 14135: Cleaned up some log and CHECK messages.
Date Mon, 16 Sep 2013 19:46:22 GMT


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 234
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line234>
> >
> >     CHECK(offers.empty())?

I like CHECK_EQ because it tells us the current size.


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 615
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line615>
> >
> >     Mention that it was discarded? In retrospect I think I should not have made
this a CHECK. Rather we should ignore it and LOG(WARNING), up to you whether you want to fix
that now.

Removed the check.


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1248
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line1248>
> >
> >     Why the leading space?

fixed


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 316
> > <https://reviews.apache.org/r/14135/diff/1/?file=352201#file352201line316>
> >
> >     CHECK(!tasks.contains(key)) or CHECK_EQ(0, tasks.count(key))?
> >     
> >     Would this benefit from including the framework id?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 328
> > <https://reviews.apache.org/r/14135/diff/1/?file=352201#file352201line328>
> >
> >     CHECK(tasks.contains(key))? Include framework id in the message?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 460
> > <https://reviews.apache.org/r/14135/diff/1/?file=352201#file352201line460>
> >
> >     framework id?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 469
> > <https://reviews.apache.org/r/14135/diff/1/?file=352201#file352201line469>
> >
> >     framework id?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 817
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line817>
> >
> >     CHECK(frameworks.contains(frameworkInfo.id()))

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1221
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line1221>
> >
> >     wrap the comment at 70 lines?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1320
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line1320>
> >
> >     Why the leading space?

copy paste error. fixed.


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/slave/gc.cpp, line 84
> > <https://reviews.apache.org/r/14135/diff/1/?file=352203#file352203line84>
> >
> >     "for gc"?

from gc


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1973
> > <https://reviews.apache.org/r/14135/diff/1/?file=352202#file352202line1973>
> >
> >     Use contains?

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/slave/paths.hpp, line 336
> > <https://reviews.apache.org/r/14135/diff/1/?file=352204#file352204line336>
> >
> >     Can we remove this altogether in favor of logging in the caller?

killed it.


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/slave/paths.hpp, line 369
> > <https://reviews.apache.org/r/14135/diff/1/?file=352204#file352204line369>
> >
> >     Can we remove this altogether in favor of logging in the caller?

killed it.


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 524
> > <https://reviews.apache.org/r/14135/diff/1/?file=352205#file352205line524>
> >
> >     Ditto on CHECK vs ignore and log, but up to you as to whether you'd like to
change it.

done


> On Sept. 16, 2013, 4:04 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1998
> > <https://reviews.apache.org/r/14135/diff/1/?file=352205#file352205line1998>
> >
> >     CHECK_NE?

done


- Vinod


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


On Sept. 14, 2013, 12:10 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14135/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2013, 12:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 19be184610fd9d75e47f44dc804afefe0babe7a6 
>   src/master/master.cpp 30abe9d4f4576a961c3d427f083fc2ce5c92b601 
>   src/slave/gc.cpp 827534f1cfbc848fce799340dd0ff8dcff3f8a11 
>   src/slave/paths.hpp 3a02a0d6a977b1af7c7f7b9d75bd4a44b0c53c2b 
>   src/slave/slave.cpp cefb42004da15d390c276ad5337b558ba5bf8e59 
> 
> Diff: https://reviews.apache.org/r/14135/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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