You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/05/22 05:29:43 UTC
Review Request 21734: MESOS-390 Handle file:// in fetcher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
Review request for mesos, Adam B and Niklas Nielsen.
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs
-----
src/launcher/fetcher.cpp 8c9e20d
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45036
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: git apply --index 21734.patch
Error:
21734.patch:210: trailing whitespace.
21734.patch:212: trailing whitespace.
21734.patch:214: trailing whitespace.
21734.patch:216: trailing whitespace.
21734.patch:223: trailing whitespace.
error: patch failed: Makefile.am:964
error: Makefile.am: patch does not apply
error: launcher/fetcher.cpp: does not exist in index
- Mesos ReviewBot
On June 8, 2014, 8:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 8:36 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45129
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
../../src/tests/fetcher_tests.cpp:73:40: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:73:40: error: in-class initialization of static data member 'ASSETS_DIRECTORY_NAME' of non-literal type
../../src/tests/fetcher_tests.cpp:74:31: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:74:31: error: in-class initialization of static data member 'COMMAND_NAME' of non-literal type
../../src/tests/fetcher_tests.cpp:76:44: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:76:44: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:76:44: error: in-class initialization of static data member 'COMMAND_SCRIPT' of non-literal type
../../src/tests/fetcher_tests.cpp:95:57: error: temporary of non-literal type 'Try<process::PID<mesos::internal::slave::Slave> >' in a constant expression
../../src/tests/fetcher_tests.cpp:95:57: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:95:57: error: in-class initialization of static data member 'slavePid' of non-literal type
../../src/tests/fetcher_tests.cpp:98:34: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:98:34: error: 'constexpr' needed for in-class initialization of static data member 'driver' of non-integral type
../../src/tests/fetcher_tests.cpp: In constructor 'FetcherTest::FetcherTest()':
../../src/tests/fetcher_tests.cpp:84:50: error: 'ASSETS_DIRECTORY_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::setupCommandFileAsset()':
../../src/tests/fetcher_tests.cpp:107:45: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:108:3: error: 'COMMAND_SCRIPT' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::startMesos()':
../../src/tests/fetcher_tests.cpp:116:3: error: 'slavePid' was not declared in this scope
../../src/tests/fetcher_tests.cpp:129:3: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::stopSlave()':
../../src/tests/fetcher_tests.cpp:141:8: error: 'slavePid' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::stopMesos()':
../../src/tests/fetcher_tests.cpp:147:3: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp:149:10: error: type '<type error>' argument given to 'delete', expected pointer
../../src/tests/fetcher_tests.cpp: In member function 'virtual void FetcherTest_FileURI_Test::TestBody()':
../../src/tests/fetcher_tests.cpp:162:32: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:167:5: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'virtual void FetcherTest_FileLocalhostURI_Test::TestBody()':
../../src/tests/fetcher_tests.cpp:229:32: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:234:3: error: 'driver' was not declared in this scope
make[4]: *** [tests/mesos_tests-fetcher_tests.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [check-am] Error 2
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 9, 2014, 6:36 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 6:36 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45189
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
../../src/tests/fetcher_tests.cpp:73:40: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:73:40: error: in-class initialization of static data member 'ASSETS_DIRECTORY_NAME' of non-literal type
../../src/tests/fetcher_tests.cpp:74:31: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:74:31: error: in-class initialization of static data member 'COMMAND_NAME' of non-literal type
../../src/tests/fetcher_tests.cpp:76:44: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:76:44: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:76:44: error: in-class initialization of static data member 'COMMAND_SCRIPT' of non-literal type
../../src/tests/fetcher_tests.cpp:95:57: error: temporary of non-literal type 'Try<process::PID<mesos::internal::slave::Slave> >' in a constant expression
../../src/tests/fetcher_tests.cpp:95:57: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:95:57: error: in-class initialization of static data member 'slavePid' of non-literal type
../../src/tests/fetcher_tests.cpp:98:34: sorry, unimplemented: non-static data member initializers
../../src/tests/fetcher_tests.cpp:98:34: error: 'constexpr' needed for in-class initialization of static data member 'driver' of non-integral type
../../src/tests/fetcher_tests.cpp: In constructor 'FetcherTest::FetcherTest()':
../../src/tests/fetcher_tests.cpp:84:50: error: 'ASSETS_DIRECTORY_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::setupCommandFileAsset()':
../../src/tests/fetcher_tests.cpp:107:45: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:108:3: error: 'COMMAND_SCRIPT' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::startMesos()':
../../src/tests/fetcher_tests.cpp:116:3: error: 'slavePid' was not declared in this scope
../../src/tests/fetcher_tests.cpp:129:3: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::stopSlave()':
../../src/tests/fetcher_tests.cpp:141:8: error: 'slavePid' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'void FetcherTest::stopMesos()':
../../src/tests/fetcher_tests.cpp:147:3: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp:149:10: error: type '<type error>' argument given to 'delete', expected pointer
../../src/tests/fetcher_tests.cpp: In member function 'virtual void FetcherTest_FileURI_Test::TestBody()':
../../src/tests/fetcher_tests.cpp:162:32: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:167:5: error: 'driver' was not declared in this scope
../../src/tests/fetcher_tests.cpp: In member function 'virtual void FetcherTest_FileLocalhostURI_Test::TestBody()':
../../src/tests/fetcher_tests.cpp:229:32: error: 'COMMAND_NAME' was not declared in this scope
../../src/tests/fetcher_tests.cpp:234:3: error: 'driver' was not declared in this scope
make[4]: *** [tests/mesos_tests-fetcher_tests.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [check-am] Error 2
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 9, 2014, 11:17 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 11:17 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 26, 2014, 8:55 a.m., Adam B wrote:
> > src/tests/fetcher_tests.cpp, line 79
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line79>
> >
> > Or do 'using mesos::internal::tests::flags'?
That still gave me reference ambigious error.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46726
-----------------------------------------------------------
On June 27, 2014, 11:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 11:24 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46726
-----------------------------------------------------------
Looks pretty good to me, but I imagine you'll have another revision coming after addressing Ian's feedback. I'll be ready to put my ShipIt stamp on it then. :)
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82283>
alphabetize
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82284>
A little more context would prevent this from looking like useless spam.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82285>
Or do 'using mesos::internal::tests::flags'?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82286>
What removes localFile at the end? Is cwd inside the sandbox that gets deleted on TearDown?
- Adam B
On June 16, 2014, 9:47 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 9:47 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 20, 2014, 8:44 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 73
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line73>
> >
> > use sandbox directory.
local file is where the file should end up being fetched to, so I wanted to use getcwd.
> On June 20, 2014, 8:44 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 79
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line79>
> >
> > aren't you using mesos::internal::tests?
I got a compile error complaining that is it ambigious, so had to add a namespace to differentiate.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46313
-----------------------------------------------------------
On June 17, 2014, 4:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 4:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 20, 2014, 8:44 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 58
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line58>
> >
> > Drop this method and use the sandbox directory as created by TemporaryDirectoryTest.
> >
> > You'll need to repeat a few lines of code for each test but it'll be cleaner and easier to follow.
I was actually thinking to create the file on another directory and verify that the file is fetched into the current working directory, so if I use sandbox for both then the file already exists. Thoughts?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46313
-----------------------------------------------------------
On June 17, 2014, 4:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 4:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Ian Downes <ia...@gmail.com>.
> On June 20, 2014, 1:44 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 58
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line58>
> >
> > Drop this method and use the sandbox directory as created by TemporaryDirectoryTest.
> >
> > You'll need to repeat a few lines of code for each test but it'll be cleaner and easier to follow.
>
> Timothy Chen wrote:
> I was actually thinking to create the file on another directory and verify that the file is fetched into the current working directory, so if I use sandbox for both then the file already exists. Thoughts?
What about having {sandbox}/from/file and verifying the file gets copied to {sandbox}/to/?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46313
-----------------------------------------------------------
On June 16, 2014, 9:47 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 9:47 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 20, 2014, 8:44 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 76
> > <https://reviews.apache.org/r/21734/diff/11/?file=611378#file611378line76>
> >
> > path::join("file://", testFile) + "+0N"?
Turns out I can't use path:join as it removes a slash in each input, so file:// will be truncated to file:/, so I'll be missing a extra slash.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46313
-----------------------------------------------------------
On June 17, 2014, 4:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 4:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46313
-----------------------------------------------------------
Thanks for simplifying the tests, much better! Almost there, just some clean ups to do.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment81618>
Shouldn't this be just "file://localhost"
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment81619>
If FILE_URI_LOCALHOST is changed as above then no need for the -1.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81636>
Remove, not needed anymore.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81637>
Remove, not needed anymore.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81638>
Remove, not needed anymore.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81621>
I don't think you neet tests/mesos.hpp now, just tests/utils.hpp for TemporaryDirectoryTest.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81620>
Drop this method and use the sandbox directory as created by TemporaryDirectoryTest.
You'll need to repeat a few lines of code for each test but it'll be cleaner and easier to follow.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81626>
Much, much simpler!
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81622>
use sandbox directory.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81635>
Add some newlines, please :-)
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81631>
path::join("file://", testFile) + "+0N"?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81624>
sandbox directory
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81634>
aren't you using mesos::internal::tests?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81629>
This should be ASSERT_SOME(fetcherProcess) otherwise you'll .get() on the next line even if isError()?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81627>
Double chevron >>?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81640>
You should check the Option<int> is some first: ASSERT_SOME(status.get()).
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81623>
ditto
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81632>
ditto path::join()?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81625>
ditto
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81630>
ditto ASSERT_
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81628>
Double chevron >> ?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment81641>
ditto ASSERT_SOME() first.
- Ian Downes
On June 16, 2014, 9:47 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 9:47 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46048
-----------------------------------------------------------
Hi Ian, I wonder if you can review my latest patch? Thanks!
- Timothy Chen
On June 17, 2014, 4:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 4:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46029
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21734]
All tests passed.
- Mesos ReviewBot
On June 17, 2014, 4:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 4:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 3e623cc
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 27, 2014, 11:27 p.m., Dominic Hamon wrote:
> > src/launcher/fetcher.cpp, line 34
> > <https://reviews.apache.org/r/21734/diff/12/?file=619949#file619949line34>
> >
> > const char FILE_URI_PREFIX[] = "file://";
> >
> >
I actually prefer it to be string as it's being used in string functions that takes string type, so boxing is needed.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46923
-----------------------------------------------------------
On June 27, 2014, 11:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 11:24 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Dominic Hamon <dh...@twopensource.com>.
> On June 27, 2014, 4:27 p.m., Dominic Hamon wrote:
> > src/launcher/fetcher.cpp, line 34
> > <https://reviews.apache.org/r/21734/diff/12/?file=619949#file619949line34>
> >
> > const char FILE_URI_PREFIX[] = "file://";
> >
> >
>
> Timothy Chen wrote:
> I actually prefer it to be string as it's being used in string functions that takes string type, so boxing is needed.
and i'd prefer it not to be because a string is non-POD :)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46923
-----------------------------------------------------------
On June 27, 2014, 4:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 4:24 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46923
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment82492>
const char FILE_URI_PREFIX[] = "file://";
- Dominic Hamon
On June 27, 2014, 4:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 4:24 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46927
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment82496>
some whitespace snuck in here.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82497>
more whitespace.
- Dominic Hamon
On June 27, 2014, 4:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 4:50 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46928
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment82498>
sorry about it, let me remove
- Timothy Chen
On June 27, 2014, 11:50 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 11:50 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46930
-----------------------------------------------------------
Ship it!
After the minor fixes below it's good!
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82502>
see below, just use getcwd(), sorry for the confusion!
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82503>
ditto
src/tests/utils.hpp
<https://reviews.apache.org/r/21734/#comment82501>
Don't expose the sandbox but do use the sandbox in the test rather than mkdtemp.
- Ian Downes
On June 27, 2014, 4:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 4:54 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46961
-----------------------------------------------------------
Ship it!
Looks great! Minor cleanup, then I'd call it ready to commit.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82531>
ASSERT_SOME(os::mkdir(fromDir));
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment82532>
ASSERT_SOME(mkdir)
src/tests/utils.hpp
<https://reviews.apache.org/r/21734/#comment82533>
Remove the extra line.
- Adam B
On June 28, 2014, 12:56 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 28, 2014, 12:56 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46943
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21734]
All tests passed.
- Mesos ReviewBot
On June 28, 2014, 7:56 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 28, 2014, 7:56 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review47021
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21734]
All tests passed.
- Mesos ReviewBot
On June 30, 2014, 9:39 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 30, 2014, 9:39 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 12d84bf
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 30, 2014, 9:39 p.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 12d84bf
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 28, 2014, 7:56 a.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 9027927
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
src/tests/utils.hpp 5c86fd4
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review46939
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21734]
All tests passed.
- Mesos ReviewBot
On June 27, 2014, 11:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 27, 2014, 11:54 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 9027927
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
> src/tests/utils.hpp 5c86fd4
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 27, 2014, 11:54 p.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 9027927
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
src/tests/utils.hpp 5c86fd4
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 27, 2014, 11:50 p.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 9027927
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
src/tests/utils.hpp 5c86fd4
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 27, 2014, 11:24 p.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 9027927
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
src/tests/utils.hpp 5c86fd4
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 17, 2014, 4:47 a.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 3e623cc
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On June 16, 2014, 10:50 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 158
> > <https://reviews.apache.org/r/21734/diff/10/?file=606960#file606960line158>
> >
> > Do you need to set up a local cluster to test the fetcher or can you test the fetcher in isolation?
> >
> > E.g., you could run the test in a temporary directory, creating files, and then running the fetcher and checking it fetches the files correctly.
> >
> > I don't see anything in the tests that requires using the full stack so these tests could be vastly simplified.
I guess technically if just testing the change alone perhaps not, just thinking to verify the fetcher works within mesos we should have unit tests in place to do so.
I can modify it so it simply just launches the fetcher passed in a file uri command if you think we don't need this for now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45850
-----------------------------------------------------------
On June 16, 2014, 8:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 8:21 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Ian Downes <ia...@gmail.com>.
> On June 16, 2014, 3:50 p.m., Ian Downes wrote:
> > src/tests/fetcher_tests.cpp, line 158
> > <https://reviews.apache.org/r/21734/diff/10/?file=606960#file606960line158>
> >
> > Do you need to set up a local cluster to test the fetcher or can you test the fetcher in isolation?
> >
> > E.g., you could run the test in a temporary directory, creating files, and then running the fetcher and checking it fetches the files correctly.
> >
> > I don't see anything in the tests that requires using the full stack so these tests could be vastly simplified.
>
> Timothy Chen wrote:
> I guess technically if just testing the change alone perhaps not, just thinking to verify the fetcher works within mesos we should have unit tests in place to do so.
> I can modify it so it simply just launches the fetcher passed in a file uri command if you think we don't need this for now.
I think it's best to just unit test the fetcher, integration testing is done elsewhere I believe.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45850
-----------------------------------------------------------
On June 16, 2014, 1:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 1:21 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45850
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment80849>
Shouldn't FILE_URI_LOCALHOST be just "localhost"? i.e., it's file:///path or file://localhost/path, in both cases it's file:// + host + /path
Would it be more explicit to have FILE_URI_LOCALHOST = "file://localhost"? this also avoids the nesting later.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment80852>
can you not use Error("")?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment80853>
I don't think the Option<string> needs to be explicitly constructed.
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/21734/#comment80851>
Do you need to set up a local cluster to test the fetcher or can you test the fetcher in isolation?
E.g., you could run the test in a temporary directory, creating files, and then running the fetcher and checking it fetches the files correctly.
I don't see anything in the tests that requires using the full stack so these tests could be vastly simplified.
- Ian Downes
On June 16, 2014, 1:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 1:21 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 16, 2014, 8:21 p.m.)
Review request for mesos and Ian Downes.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs
-----
src/Makefile.am c91b438
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45417
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Assembler messages:
Error: can't open /tmp/cc4FaIFf.s for reading: No such file or directory
make[4]: *** [tests/mesos_tests-sasl_tests.o] Error 1
make[4]: *** Waiting for unfinished jobs....
Assembler messages:
Error: can't open /tmp/ccpqG9QZ.s for reading: No such file or directory
make[4]: *** [tests/mesos_tests-resource_offers_tests.o] Error 1
Assembler messages:
Error: can't open /tmp/ccY8edbq.s for reading: No such file or directory
make[4]: *** [tests/mesos_tests-scheduler_tests.o] Error 1
make[3]: *** [check-am] Error 2
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 11, 2014, 3:41 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 3:41 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 11, 2014, 3:41 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
rebased
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am c91b438
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45323
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: make check
Error:
None
- Mesos ReviewBot
On June 10, 2014, 4:50 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 4:50 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 10, 2014, 4:50 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 4a3f2e1
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 9, 2014, 11:17 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 4a3f2e1
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45103
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment79770>
Mind introducing a const string for "file://" and do a .length()/.size() here? Magic numbers are error prone.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment79771>
Same here.
- Niklas Nielsen
On June 9, 2014, 11:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 11:36 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 9, 2014, 6:36 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 4a3f2e1
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review45041
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: git apply --index 21734.patch
Error:
error: patch failed: Makefile.am:964
error: Makefile.am: patch does not apply
error: launcher/fetcher.cpp: does not exist in index
- Mesos ReviewBot
On June 8, 2014, 5:19 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 5:19 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 8, 2014, 5:19 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
removing trailing spaces
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 4a3f2e1
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 8, 2014, 8:36 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am 4a3f2e1
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review44983
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: git apply --index 21734.patch
Error:
21734.patch:210: trailing whitespace.
21734.patch:212: trailing whitespace.
21734.patch:214: trailing whitespace.
21734.patch:216: trailing whitespace.
21734.patch:223: trailing whitespace.
error: patch failed: Makefile.am:957
error: Makefile.am: patch does not apply
error: launcher/fetcher.cpp: does not exist in index
- Mesos ReviewBot
On June 6, 2014, 12:20 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 12:20 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-390
> https://issues.apache.org/jira/browse/MESOS-390
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/Makefile.am c3ecb94
> src/launcher/fetcher.cpp c4425eb
> src/tests/fetcher_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
> Added unit tests to test file:// and file://localhost.
> make check.
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 6, 2014, 12:20 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-390
https://issues.apache.org/jira/browse/MESOS-390
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs
-----
src/Makefile.am c3ecb94
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing (updated)
-------
Added unit tests to test file:// and file://localhost.
make check.
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 6, 2014, 12:20 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am c3ecb94
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/
-----------------------------------------------------------
(Updated June 5, 2014, 9:21 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
Added unit tests and addressed comments.
Repository: mesos-git
Description
-------
Handling file:// in the fetcher, and also handle case when localhost is used.
Diffs (updated)
-----
src/Makefile.am c3ecb94
src/launcher/fetcher.cpp c4425eb
src/tests/fetcher_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/21734/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review44297
-----------------------------------------------------------
Bad patch!
Reviews applied: [21734]
Failed command: git apply --index 21734.patch
Error:
error: launcher/fetcher.cpp: does not exist in index
- Mesos ReviewBot
On May 22, 2014, 3:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 3:29 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8c9e20d
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On May 28, 2014, 10:47 p.m., Adam B wrote:
> > src/launcher/fetcher.cpp, lines 141-142
> > <https://reviews.apache.org/r/21734/diff/2/?file=587748#file587748line141>
> >
> > Can you LOG(ERROR) here too?
> > Is there a reason you need to do this check here rather than rely on the local.find_first_of("/")!=0 below?
> > Is one of these checks better than the other? I would think that startsWith would be able to exit earlier (after 1st char) than find_first_of (after 1st '/'). Perhaps we should change it too.
The reason I did an explicit check here is because technically File URI in linux doesn't support relative paths, so I want to keep the same semantics.
The code below that is actually prepending MESOS_FRAMEWORKS_HOME if it's a relative path.
I think we shouldn't allow relative paths given a file://.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review44187
-----------------------------------------------------------
On May 22, 2014, 3:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 3:29 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8c9e20d
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review44187
-----------------------------------------------------------
Looks pretty good. Please also add "MESOS-390" to the "Bugs" field, and update the "Testing Done" field with at least 'make check', so we know you built and tested with your changes. As Niklas suggested, we will probably want a unit test for this change, but maybe it makes sense to ask Bernd how it would fit in with his new fetcher tests.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment78512>
Why use find("://") + 3 if you already know that it starts with "file://"? Can't you just use local.substr(7)?
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/21734/#comment78514>
Can you LOG(ERROR) here too?
Is there a reason you need to do this check here rather than rely on the local.find_first_of("/")!=0 below?
Is one of these checks better than the other? I would think that startsWith would be able to exit earlier (after 1st char) than find_first_of (after 1st '/'). Perhaps we should change it too.
- Adam B
On May 21, 2014, 8:29 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 8:29 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8c9e20d
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Timothy Chen <tn...@apache.org>.
> On May 22, 2014, 3:44 a.m., Niklas Nielsen wrote:
> > Super cool Tim! The code looks great but would love to see a test for this :) Should be doable for local files, but let's sync up if you need some input on how to get going.
That was originally the plan :) However I know Bernd created a nice framework for fetcher test for the feature he's working on, so I was thinking to use what he did.
Chatted with Adam and thought I'll just post this first and see as it's not a big change.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review43700
-----------------------------------------------------------
On May 22, 2014, 3:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 3:29 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8c9e20d
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 21734: MESOS-390 Handle file:// in fetcher
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21734/#review43700
-----------------------------------------------------------
Super cool Tim! The code looks great but would love to see a test for this :) Should be doable for local files, but let's sync up if you need some input on how to get going.
- Niklas Nielsen
On May 21, 2014, 8:29 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21734/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 8:29 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Handling file:// in the fetcher, and also handle case when localhost is used.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8c9e20d
>
> Diff: https://reviews.apache.org/r/21734/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>