You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/07/20 06:41:10 UTC
Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/
-----------------------------------------------------------
Review request for mesos, John Sirois and Vinod Kone.
Description
-------
See summary.
Diffs
-----
src/logging/logging.cpp 074765f
third_party/libprocess/include/stout/time.hpp 4819b9e
third_party/libprocess/src/process.cpp b560b7c
Diff: https://reviews.apache.org/r/6056/diff/
Testing
-------
make check
And manually via:
$ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
Thanks,
Benjamin Hindman
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 20, 2012, 3:49 p.m., John Sirois wrote:
> > src/logging/logging.cpp, line 136
> > <https://reviews.apache.org/r/6056/diff/3/?file=125525#file125525line136>
> >
> > So is __sync_synchronize a gflags symbol or do you happen to know it also uses a read barrier?
__sync_synchronize is a gcc "intrinsic/builtin". I don't really have any "ordering" issues to worry about, so I mostly put these in as a reminder to the compiler (and myself) that I want this write to be visible by other threads immediately (i.e., not stashed in a temporary/register and flushed to memory later).
For single variables like this, x86 cache coherence takes care of most things, it's the compiler that can trip you up. On the read side, if we were sitting in a tight loop constantly reading the variable it's likely we'd need a __sync_synchronize to tell the compiler not to keep reading from the temporary/register that it loaded the variable into. But that's not how these variables get read by glog, so I'm not too concerned. Yes, I'm really only thinking about x86 (and ARM, I guess). ;)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/#review9312
-----------------------------------------------------------
On July 20, 2012, 6:26 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6056/
> -----------------------------------------------------------
>
> (Updated July 20, 2012, 6:26 a.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/logging/logging.cpp 074765f
> third_party/libprocess/include/process/http.hpp 2ef8a3d
> third_party/libprocess/include/stout/time.hpp 4819b9e
> third_party/libprocess/src/process.cpp b560b7c
>
> Diff: https://reviews.apache.org/r/6056/diff/
>
>
> Testing
> -------
>
> make check
>
> And manually via:
>
> $ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/#review9312
-----------------------------------------------------------
Ship it!
src/logging/logging.cpp
<https://reviews.apache.org/r/6056/#comment19930>
So is __sync_synchronize a gflags symbol or do you happen to know it also uses a read barrier?
- John Sirois
On July 20, 2012, 6:26 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6056/
> -----------------------------------------------------------
>
> (Updated July 20, 2012, 6:26 a.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/logging/logging.cpp 074765f
> third_party/libprocess/include/process/http.hpp 2ef8a3d
> third_party/libprocess/include/stout/time.hpp 4819b9e
> third_party/libprocess/src/process.cpp b560b7c
>
> Diff: https://reviews.apache.org/r/6056/diff/
>
>
> Testing
> -------
>
> make check
>
> And manually via:
>
> $ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/#review9300
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On July 20, 2012, 6:26 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6056/
> -----------------------------------------------------------
>
> (Updated July 20, 2012, 6:26 a.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/logging/logging.cpp 074765f
> third_party/libprocess/include/process/http.hpp 2ef8a3d
> third_party/libprocess/include/stout/time.hpp 4819b9e
> third_party/libprocess/src/process.cpp b560b7c
>
> Diff: https://reviews.apache.org/r/6056/diff/
>
>
> Testing
> -------
>
> make check
>
> And manually via:
>
> $ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/
-----------------------------------------------------------
(Updated July 20, 2012, 6:26 a.m.)
Review request for mesos, John Sirois and Vinod Kone.
Changes
-------
Fixed a bug from the last update.
Description
-------
See summary.
Diffs (updated)
-----
src/logging/logging.cpp 074765f
third_party/libprocess/include/process/http.hpp 2ef8a3d
third_party/libprocess/include/stout/time.hpp 4819b9e
third_party/libprocess/src/process.cpp b560b7c
Diff: https://reviews.apache.org/r/6056/diff/
Testing
-------
make check
And manually via:
$ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
Thanks,
Benjamin Hindman
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/
-----------------------------------------------------------
(Updated July 20, 2012, 6:24 a.m.)
Review request for mesos, John Sirois and Vinod Kone.
Changes
-------
Added reusable body/header code to process::http::Response and used it in this review.
Description
-------
See summary.
Diffs (updated)
-----
src/logging/logging.cpp 074765f
third_party/libprocess/include/process/http.hpp 2ef8a3d
third_party/libprocess/include/stout/time.hpp 4819b9e
third_party/libprocess/src/process.cpp b560b7c
Diff: https://reviews.apache.org/r/6056/diff/
Testing
-------
make check
And manually via:
$ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
Thanks,
Benjamin Hindman
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 20, 2012, 6:21 a.m., Vinod Kone wrote:
> > src/logging/logging.cpp, line 56
> > <https://reviews.apache.org/r/6056/diff/1/?file=125516#file125516line56>
> >
> > s/original/default/ ?
I'd love to, but 'default' is a reserved word (used in 'switch' statements).
> On July 20, 2012, 6:21 a.m., Vinod Kone wrote:
> > src/logging/logging.cpp, line 70
> > <https://reviews.apache.org/r/6056/diff/1/?file=125516#file125516line70>
> >
> > why capital T?
'This' is a typedef to the current process type. Also, it's not legal to do '&this::toggle'.
> On July 20, 2012, 6:21 a.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/time.hpp, line 192
> > <https://reviews.apache.org/r/6056/diff/1/?file=125517#file125517line192>
> >
> > would be great to have support decimal values
Updated the TODO.
> On July 20, 2012, 6:21 a.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/time.hpp, line 197
> > <https://reviews.apache.org/r/6056/diff/1/?file=125517#file125517line197>
> >
> > why double? from the above, you are only expecting integral value?
For when I eventually support decimals! ;)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/#review9296
-----------------------------------------------------------
On July 20, 2012, 6:26 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6056/
> -----------------------------------------------------------
>
> (Updated July 20, 2012, 6:26 a.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/logging/logging.cpp 074765f
> third_party/libprocess/include/process/http.hpp 2ef8a3d
> third_party/libprocess/include/stout/time.hpp 4819b9e
> third_party/libprocess/src/process.cpp b560b7c
>
> Diff: https://reviews.apache.org/r/6056/diff/
>
>
> Testing
> -------
>
> make check
>
> And manually via:
>
> $ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added a mechanism for dynamically toggling the verbose
logging level.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6056/#review9296
-----------------------------------------------------------
src/logging/logging.cpp
<https://reviews.apache.org/r/6056/#comment19917>
s/original/default/ ?
src/logging/logging.cpp
<https://reviews.apache.org/r/6056/#comment19918>
why capital T?
third_party/libprocess/include/stout/time.hpp
<https://reviews.apache.org/r/6056/#comment19919>
would be great to have support decimal values
third_party/libprocess/include/stout/time.hpp
<https://reviews.apache.org/r/6056/#comment19920>
why double? from the above, you are only expecting integral value?
- Vinod Kone
On July 20, 2012, 4:41 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6056/
> -----------------------------------------------------------
>
> (Updated July 20, 2012, 4:41 a.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/logging/logging.cpp 074765f
> third_party/libprocess/include/stout/time.hpp 4819b9e
> third_party/libprocess/src/process.cpp b560b7c
>
> Diff: https://reviews.apache.org/r/6056/diff/
>
>
> Testing
> -------
>
> make check
>
> And manually via:
>
> $ curl 'localhost:5050/logging/toggle?level=3&duration=10secs'
>
>
> Thanks,
>
> Benjamin Hindman
>
>