You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/04/27 02:45:07 UTC
Review Request: Extended utils
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
Review request for mesos and John Sirois.
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
> On 2012-04-27 01:11:43, John Sirois wrote:
> > src/common/protobuf_utils.hpp, line 52
> > <https://reviews.apache.org/r/4899/diff/2/?file=104741#file104741line52>
> >
> > const reference for this guy too right?
fixed
> On 2012-04-27 01:11:43, John Sirois wrote:
> > src/common/utils.hpp, line 190
> > <https://reviews.apache.org/r/4899/diff/2/?file=104742#file104742line190>
> >
> > Tests for these seem in order. IIRC there is a decent temp dir utility to setup files and dirs for this.
added.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7290
-----------------------------------------------------------
On 2012-04-27 01:11:30, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-04-27 01:11:30)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp b233b68
> src/tests/protobuf_io_tests.cpp 22f37ac
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7290
-----------------------------------------------------------
Stopped short at utils.hpp for lack of tests.
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/4899/#comment16096>
const reference for this guy too right?
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16097>
Tests for these seem in order. IIRC there is a decent temp dir utility to setup files and dirs for this.
- John
On 2012-04-27 01:11:30, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-04-27 01:11:30)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp b233b68
> src/tests/protobuf_io_tests.cpp 22f37ac
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review8024
-----------------------------------------------------------
Ship it!
Coolio, I'll get this submitted!
- Benjamin
On 2012-05-18 17:06:20, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-05-18 17:06:20)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp 09a8396
> src/tests/protobuf_io_tests.cpp 22f37ac
> src/tests/utils_tests.cpp 49ec107
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
(Updated 2012-05-18 17:06:20.043811)
Review request for mesos and John Sirois.
Changes
-------
ping for review!
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/slave/slave.cpp 09a8396
src/tests/protobuf_io_tests.cpp 22f37ac
src/tests/utils_tests.cpp 49ec107
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
(Updated 2012-05-11 00:59:54.273443)
Review request for mesos and John Sirois.
Changes
-------
ben's comments
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs (updated)
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/slave/slave.cpp 09a8396
src/tests/protobuf_io_tests.cpp 22f37ac
src/tests/utils_tests.cpp 49ec107
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 190
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line190>
> >
> > Related to the previous comment, I don't like something called create that also truncates.
> >
> > If the use case is to have a new truncated file, one can simply do:
> >
> > utils::os::rm(path);
> > utils::os::touch(path);
> >
> > That is, I'd prefer this was the 'touch' abstraction we know and love, without the truncate. I would consider the ability to pass a 'bool truncate' flag to 'touch', but I'm not sure it's necessary.
ok..removed O_TRUNC option and renamed create to touch.
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 196
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line196>
> >
> > We really shouldn't be logging here (and everywhere else in utils, since they all return Try or Result).
killed. here and everywhere else
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 217
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line217>
> >
> > Shouldn't need this line, just use 'strerror(errno)' on the next line.
done
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 253
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line253>
> >
> > Okay, I reneg my previous comment. I think it makes sense to use Result for read because there are three possibilities: (1) the data; (2) an error; (3) EOF. We can represent EOF as Result::none.
fixed for all versions of read(). do we want Try or Result for write(). Technically, write shouldnt return a none() but I like the symmetry.
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 265
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line265>
> >
> > Did you want an 'else if' here? And you should kill this check anyway and return a Result::none below where you check if length == 0.
killed
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 303
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line303>
> >
> > Kill.
killed
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 304
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line304>
> >
> > s/open file./open " + path
done. here and everywhere else.
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 543
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line543>
> >
> > Kill.
killed
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 555
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line555>
> >
> > What if it's an error? If your intention is that it will never be an error, make this a CHECK first before you do a .get().
added the check.
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 748
> > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line748>
> >
> > Right, here, using Result makes sense (like the read above). Just all the other places didn't make sense.
fixed
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 737
> > <https://reviews.apache.org/r/4899/diff/5/?file=106625#file106625line737>
> >
> > s/rea/Try<
:)
> On 2012-05-08 23:16:57, Benjamin Hindman wrote:
> > src/tests/utils_tests.cpp, line 89
> > <https://reviews.apache.org/r/4899/diff/5/?file=106627#file106627line89>
> >
> > UtilsTest should really be a fixture which makes sure the file gets deleted even if the test fails (see https://reviews.apache.org/r/4981 for an example).
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7702
-----------------------------------------------------------
On 2012-05-04 00:28:13, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-05-04 00:28:13)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp 09a8396
> src/tests/protobuf_io_tests.cpp 22f37ac
> src/tests/utils_tests.cpp 49ec107
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7702
-----------------------------------------------------------
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16951>
Related to the previous comment, I don't like something called create that also truncates.
If the use case is to have a new truncated file, one can simply do:
utils::os::rm(path);
utils::os::touch(path);
That is, I'd prefer this was the 'touch' abstraction we know and love, without the truncate. I would consider the ability to pass a 'bool truncate' flag to 'touch', but I'm not sure it's necessary.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16952>
We really shouldn't be logging here (and everywhere else in utils, since they all return Try or Result).
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16953>
Shouldn't need this line, just use 'strerror(errno)' on the next line.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16959>
Okay, I reneg my previous comment. I think it makes sense to use Result for read because there are three possibilities: (1) the data; (2) an error; (3) EOF. We can represent EOF as Result::none.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16954>
Did you want an 'else if' here? And you should kill this check anyway and return a Result::none below where you check if length == 0.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16961>
Kill.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16962>
s/open file./open " + path
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16960>
Kill.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16963>
What if it's an error? If your intention is that it will never be an error, make this a CHECK first before you do a .get().
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16964>
Right, here, using Result makes sense (like the read above). Just all the other places didn't make sense.
src/slave/slave.cpp
<https://reviews.apache.org/r/4899/#comment16965>
s/rea/Try<
src/tests/utils_tests.cpp
<https://reviews.apache.org/r/4899/#comment16966>
UtilsTest should really be a fixture which makes sure the file gets deleted even if the test fails (see https://reviews.apache.org/r/4981 for an example).
- Benjamin
On 2012-05-04 00:28:13, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-05-04 00:28:13)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp 09a8396
> src/tests/protobuf_io_tests.cpp 22f37ac
> src/tests/utils_tests.cpp 49ec107
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
(Updated 2012-05-04 00:28:13.663437)
Review request for mesos and John Sirois.
Changes
-------
john's commnets
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs (updated)
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/slave/slave.cpp 09a8396
src/tests/protobuf_io_tests.cpp 22f37ac
src/tests/utils_tests.cpp 49ec107
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 192
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line192>
> >
> > touch doesn't truncate - this probably should not either ... or else the name should change.
agreed. s/touch/create
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 208
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line208>
> >
> > You copied local style as in the inlined open method above, but I suspect that was itself a typo based on other inline functions in this file and other files in mesos. -2 indent here and elsewhere on function opening { and closing } ?
yea..stupid eclipse auto formatting. fixed
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 222
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line222>
> >
> > You own this TODO
actually, i just copied this from #707
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 255
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line255>
> >
> > SEEK_SET or should the doc be ammended to say read the file from its current offset to the end?
amended the doc
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 263
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line263>
> >
> > This could error as well as the 2 lseeks above, seems like that should be handled and propagated via the Try return type.
fixed
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 521
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line521>
> >
> > You should s/pattern/prefix/ - afaict thats all you will successfully match.
its not just the prefix, it could match anywhere in the file name.
> On 2012-05-03 22:28:33, John Sirois wrote:
> > src/common/utils.hpp, line 545
> > <https://reviews.apache.org/r/4899/diff/4/?file=106486#file106486line545>
> >
> > Instead of a recursive function, why not leverage ftw and even fnmatch if you want some globbiness.
is your concern that the stack might get too deep or something else?
talked to ben offline about using ftw. since ftw uses function pointers, we would need to move the find() to utils.cpp. we are trying to get rid of utils.cpp to make it easier for third-parties to use our utils!
added a TODO for now.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7518
-----------------------------------------------------------
On 2012-05-03 19:23:34, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-05-03 19:23:34)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp b233b68
> src/tests/protobuf_io_tests.cpp 22f37ac
> src/tests/utils_tests.cpp 49ec107
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/#review7518
-----------------------------------------------------------
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16644>
touch doesn't truncate - this probably should not either ... or else the name should change.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16642>
You copied local style as in the inlined open method above, but I suspect that was itself a typo based on other inline functions in this file and other files in mesos. -2 indent here and elsewhere on function opening { and closing } ?
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16646>
You own this TODO
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16647>
SEEK_SET or should the doc be ammended to say read the file from its current offset to the end?
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16648>
This could error as well as the 2 lseeks above, seems like that should be handled and propagated via the Try return type.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16650>
You should s/pattern/prefix/ - afaict thats all you will successfully match.
src/common/utils.hpp
<https://reviews.apache.org/r/4899/#comment16661>
Instead of a recursive function, why not leverage ftw and even fnmatch if you want some globbiness.
- John
On 2012-05-03 19:23:34, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4899/
> -----------------------------------------------------------
>
> (Updated 2012-05-03 19:23:34)
>
>
> Review request for mesos and John Sirois.
>
>
> Summary
> -------
>
> We can now read/write strings to a file.
>
> Some common utils for working with protobuf objects.
>
>
> Diffs
> -----
>
> src/Makefile.am cd503a8
> src/common/protobuf_utils.hpp PRE-CREATION
> src/common/utils.hpp 1d81e21
> src/slave/slave.cpp b233b68
> src/tests/protobuf_io_tests.cpp 22f37ac
> src/tests/utils_tests.cpp 49ec107
>
> Diff: https://reviews.apache.org/r/4899/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
(Updated 2012-05-03 19:23:34.846482)
Review request for mesos and John Sirois.
Changes
-------
added test cases.
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs (updated)
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/slave/slave.cpp b233b68
src/tests/protobuf_io_tests.cpp 22f37ac
src/tests/utils_tests.cpp 49ec107
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Extended utils
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4899/
-----------------------------------------------------------
(Updated 2012-04-27 01:11:30.771867)
Review request for mesos and John Sirois.
Changes
-------
s/Result/Try in utils.hpp!
Summary
-------
We can now read/write strings to a file.
Some common utils for working with protobuf objects.
Diffs (updated)
-----
src/Makefile.am cd503a8
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/slave/slave.cpp b233b68
src/tests/protobuf_io_tests.cpp 22f37ac
Diff: https://reviews.apache.org/r/4899/diff
Testing
-------
make check
Thanks,
Vinod