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/05/01 04:40:13 UTC
Review Request: Refactored path utilities into a separate file
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and John Sirois.
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs
-----
src/Makefile.am cd503a8
src/common/utils.hpp 1d81e21
src/slave/path.hpp PRE-CREATION
src/slave/slave.hpp 0dc7140
src/slave/slave.cpp b233b68
src/tests/path_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
(Updated 2012-06-02 00:16:01.231733)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
fixed test.
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs (updated)
-----
src/Makefile.am 8760f59
src/common/protobuf_utils.hpp PRE-CREATION
src/common/utils.hpp ba55497
src/slave/path.hpp PRE-CREATION
src/slave/path.cpp PRE-CREATION
src/slave/slave.hpp 80453b0
src/slave/slave.cpp aac11e0
src/tests/path_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
(Updated 2012-05-23 23:23:42.286812)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
ben's comments.
haven't run make check due to a compilation error, that I'm having difficulty figuring out (help?).
Undefined symbols:
"std::basic_string<char, std::char_traits<char>, std::allocator<char> > mesos::internal::slave::path::format<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, mesos::SlaveID, mesos::FrameworkID, mesos::ExecutorID, int, mesos::TaskID>(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, mesos::SlaveID, mesos::FrameworkID, mesos::ExecutorID, int, mesos::TaskID)", referenced from:
mesos::internal::slave::path::PathTestFixture_format_Test::TestBody() in mesos_tests-path_tests.o
"std::basic_string<char, std::char_traits<char>, std::allocator<char> > mesos::internal::slave::path::format<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, mesos::SlaveID, mesos::FrameworkID, mesos::ExecutorID>(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, mesos::SlaveID, mesos::FrameworkID, mesos::ExecutorID)", referenced from:
mesos::internal::slave::path::PathTestFixture_format_Test::TestBody() in mesos_tests-path_tests.o
ld: symbol(s) not found
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs (updated)
-----
src/tests/protobuf_io_tests.cpp 22f37ac
src/tests/utils_tests.cpp 49ec107
src/tests/path_tests.cpp PRE-CREATION
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp e08be3b
src/master/http.cpp f72ceb7
src/slave/path.hpp PRE-CREATION
src/slave/path.cpp PRE-CREATION
src/common/utils.hpp 1d81e21
src/Makefile.am 333234d
src/common/protobuf_utils.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 54
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line54>
> >
> > Now that we're using ostringstream we should include 'sstream'.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 229
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line229>
> >
> > Why not call this mktemp? Also, spaces around '=' please.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 231
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line231>
> >
> > Wait, 'rootDir' is not a dir, it's a path, let's call it 'path'.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 236
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line236>
> >
> > Why not include strerror here?
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 415
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line415>
> >
> > s/mktempdir/mkdtemp
> > s/t="/t = "
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 417
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line417>
> >
> > Now it's a directory, but you might as well still call it path. ;)
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 422
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line422>
> >
> > And strerror would again be helpful.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 510
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line510>
> >
> > s/reported/reported.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 700
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line700>
> >
> > const std::string&
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 705
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line705>
> >
> > Formatting is wrong here.
fixed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/common/utils.hpp, line 716
> > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line716>
> >
> > Formatting.
fixed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 38
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line38>
> >
> > I think the suffix _PATH reads better to me. Likewise, I think either just 'get' or 'format' reads better than 'getPath' as well. For example, the following two lines read pretty naturally to me:
> >
> > get(FRAMEWORK_PATH, root, slaveId, frameworkId)
> > format(FRAMEWORK_PATH, root, slaveId, frameworkId)
s/_LAYOUT/_PATH/
s/getPath/format/
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 56
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line56>
> >
> > Please reformat all opening '{' in this file.
done.
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 57
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line57>
> >
> > Why not call this enum 'Type'? Seems more descriptive than 'Data'.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 68
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line68>
> >
> > Kill space.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 63
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line63>
> >
> > Wait, aren't there only two files? Do we really need a hashmap?
hashmap makes it easy when we lookup a specific type of file.
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 77
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line77>
> >
> > Again, aren't there only three files? Are these supposed to be the path to these files?
yes. updated the doc to reflect this.
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 85
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line85>
> >
> > I think 'latest' is more descriptive than maxrun. I also think the cleanest way to do this would really be, when someone wants the latest run:
> >
> > RunState state;
> >
> > int latest = sort(state.runs.keys()).back();
> >
> > But perhaps that's still not possible given our current libraries. *sigh*
s/maxrun/latest
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 100
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line100>
> >
> > Space after ',' please.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 107
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line107>
> >
> > I think you should call this 'get' or even just 'format', since that's what it will ultimately get called once we move the format stuff into strings.
calling it format
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.hpp, line 144
> > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line144>
> >
> > Are these duplicated in the .cpp? Either way, we shouldn't keep them here.
killed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 22
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line22>
> >
> > Why not:
> >
> > utils::os::glob(getPath(FRAMEWORK_LAYOUT, root, slaveId, '*'));
> >
> > You can put that all on a newline.
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 23
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line23>
> >
> > Formatting.
fixed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 40
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line40>
> >
> > Again, it would be better to use EXECUTOR_LAYOUT here like above so as to keep the semantics encoded in the constants.
fixed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 41
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line41>
> >
> > Formatting!
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 51
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line51>
> >
> > For ma tt ing.
fixed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 58
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line58>
> >
> > Maybe just skip instead of fail?
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 97
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line97>
> >
> > "No tasks found for run " << run << " of executor " << executorId << " of framework " << frameworkId
done
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 108
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line108>
> >
> > Again, it seems like recording the TaskID is the most important part, and getting the paths to these files can be done later, wherever it is needed.
agree, killed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 71
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line71>
> >
> > Seeing this done here like this makes me wonder why it can't be done later, when it's needed. (For the others too.)
good point, killed
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/slave/path.cpp, line 206
> > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line206>
> >
> > What about something like this here instead:
> >
> > int run = 0;
> >
> > Result<list<string> > paths = glob(format(EXECUTOR_PATH, directory, id, frameworkId, executorId, '*'));
> >
> > CHECK(!paths.isError()) << paths.error();
> >
> > if (paths.isSome()) {
> > foreach (const string& path, paths.get()) {
> > Try<int> temp = numify<int>(basename(path));
> > if (temp.isError()) {
> > continue;
> > }
> > run = max(run, temp.get());
> > }
> > }
> >
> > string result = format(EXECUTOR_PATH, directory, id, frameworkId, executorId, run + 1);
> >
> > bool created = mkdir(result);
> >
> > CHECK(created) << ...;
> >
> > return result;
thanx for the code :) replaced w/ minor modifications.
> On 2012-05-21 23:34:36, Benjamin Hindman wrote:
> > src/tests/path_tests.cpp, line 44
> > <https://reviews.apache.org/r/4944/diff/4/?file=109109#file109109line44>
> >
> > If the rootDir is not changing across tests, why not set it up for the entire fixture rather than for each test?
it is changing...it is created by mktemp
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/#review8021
-----------------------------------------------------------
On 2012-05-18 17:05:42, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4944/
> -----------------------------------------------------------
>
> (Updated 2012-05-18 17:05:42)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Summary
> -------
>
> See summary.
>
> This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
>
> Added tests.
>
>
> Diffs
> -----
>
> src/tests/path_tests.cpp PRE-CREATION
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp e08be3b
> src/slave/path.cpp PRE-CREATION
> src/master/http.cpp f72ceb7
> src/slave/path.hpp PRE-CREATION
> src/Makefile.am 333234d
> src/common/utils.hpp 1d81e21
>
> Diff: https://reviews.apache.org/r/4944/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Refactored path utilities into a separate file
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/#review8021
-----------------------------------------------------------
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17419>
Now that we're using ostringstream we should include 'sstream'.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17420>
Why not call this mktemp? Also, spaces around '=' please.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17421>
Wait, 'rootDir' is not a dir, it's a path, let's call it 'path'.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17422>
Why not include strerror here?
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17423>
s/mktempdir/mkdtemp
s/t="/t = "
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17424>
Now it's a directory, but you might as well still call it path. ;)
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17425>
And strerror would again be helpful.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17428>
s/reported/reported.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17426>
const std::string&
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17427>
Formatting is wrong here.
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17429>
Good use of Result!
src/common/utils.hpp
<https://reviews.apache.org/r/4944/#comment17430>
Formatting.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17437>
I think the suffix _PATH reads better to me. Likewise, I think either just 'get' or 'format' reads better than 'getPath' as well. For example, the following two lines read pretty naturally to me:
get(FRAMEWORK_PATH, root, slaveId, frameworkId)
format(FRAMEWORK_PATH, root, slaveId, frameworkId)
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17440>
Please reformat all opening '{' in this file.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17439>
Why not call this enum 'Type'? Seems more descriptive than 'Data'.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17441>
Wait, aren't there only two files? Do we really need a hashmap?
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17431>
Kill space.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17442>
Again, aren't there only three files? Are these supposed to be the path to these files?
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17443>
I think 'latest' is more descriptive than maxrun. I also think the cleanest way to do this would really be, when someone wants the latest run:
RunState state;
int latest = sort(state.runs.keys()).back();
But perhaps that's still not possible given our current libraries. *sigh*
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17444>
Space after ',' please.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17438>
I think you should call this 'get' or even just 'format', since that's what it will ultimately get called once we move the format stuff into strings.
src/slave/path.hpp
<https://reviews.apache.org/r/4944/#comment17432>
Are these duplicated in the .cpp? Either way, we shouldn't keep them here.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17435>
Why not:
utils::os::glob(getPath(FRAMEWORK_LAYOUT, root, slaveId, '*'));
You can put that all on a newline.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17433>
Formatting.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17434>
This is great: the way this is being done "procedurally" now enables great log lines like this!
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17446>
Again, it would be better to use EXECUTOR_LAYOUT here like above so as to keep the semantics encoded in the constants.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17445>
Formatting!
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17447>
For ma tt ing.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17436>
Maybe just skip instead of fail?
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17448>
Seeing this done here like this makes me wonder why it can't be done later, when it's needed. (For the others too.)
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17449>
"No tasks found for run " << run << " of executor " << executorId << " of framework " << frameworkId
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17450>
Again, it seems like recording the TaskID is the most important part, and getting the paths to these files can be done later, wherever it is needed.
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17451>
What about something like this here instead:
int run = 0;
Result<list<string> > paths = glob(format(EXECUTOR_PATH, directory, id, frameworkId, executorId, '*'));
CHECK(!paths.isError()) << paths.error();
if (paths.isSome()) {
foreach (const string& path, paths.get()) {
Try<int> temp = numify<int>(basename(path));
if (temp.isError()) {
continue;
}
run = max(run, temp.get());
}
}
string result = format(EXECUTOR_PATH, directory, id, frameworkId, executorId, run + 1);
bool created = mkdir(result);
CHECK(created) << ...;
return result;
src/slave/path.cpp
<https://reviews.apache.org/r/4944/#comment17453>
These are fine for now but if they are only being used in one place my preference will be that this code just get's inlined there.
src/slave/slave.cpp
<https://reviews.apache.org/r/4944/#comment17454>
Glad to see all of this go! It's been wreaking havoc on my ability to sleep well at night. ;)
src/tests/path_tests.cpp
<https://reviews.apache.org/r/4944/#comment17455>
If the rootDir is not changing across tests, why not set it up for the entire fixture rather than for each test?
- Benjamin
On 2012-05-18 17:05:42, Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4944/
> -----------------------------------------------------------
>
> (Updated 2012-05-18 17:05:42)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Summary
> -------
>
> See summary.
>
> This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
>
> Added tests.
>
>
> Diffs
> -----
>
> src/tests/path_tests.cpp PRE-CREATION
> src/slave/slave.hpp 08a29d8
> src/slave/slave.cpp e08be3b
> src/slave/path.cpp PRE-CREATION
> src/master/http.cpp f72ceb7
> src/slave/path.hpp PRE-CREATION
> src/Makefile.am 333234d
> src/common/utils.hpp 1d81e21
>
> Diff: https://reviews.apache.org/r/4944/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod
>
>
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
(Updated 2012-05-18 17:05:42.762269)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
ping for review!
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs
-----
src/tests/path_tests.cpp PRE-CREATION
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp e08be3b
src/slave/path.cpp PRE-CREATION
src/master/http.cpp f72ceb7
src/slave/path.hpp PRE-CREATION
src/Makefile.am 333234d
src/common/utils.hpp 1d81e21
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
(Updated 2012-05-15 22:48:01.291108)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
minor fix for http.cpp
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs (updated)
-----
src/tests/path_tests.cpp PRE-CREATION
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp e08be3b
src/slave/path.cpp PRE-CREATION
src/master/http.cpp f72ceb7
src/slave/path.hpp PRE-CREATION
src/Makefile.am 333234d
src/common/utils.hpp 1d81e21
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod
Re: Review Request: Refactored path utilities into a separate file
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4944/
-----------------------------------------------------------
(Updated 2012-05-15 22:37:50.881276)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
new refactoring of path to use path templates.
also added glob to utils.
Summary
-------
See summary.
This diff is based on the previous utils patch (https://reviews.apache.org/r/4899). I still need to update that patch with john's comments.
Added tests.
Diffs (updated)
-----
src/tests/path_tests.cpp PRE-CREATION
src/slave/slave.hpp 08a29d8
src/slave/slave.cpp e08be3b
src/slave/path.cpp PRE-CREATION
src/slave/path.hpp PRE-CREATION
src/Makefile.am 333234d
src/common/utils.hpp 1d81e21
Diff: https://reviews.apache.org/r/4944/diff
Testing
-------
make check
Thanks,
Vinod