You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/08/15 04:48:12 UTC
Review Request: Exposing files through http endpoints
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs
-----
src/Makefile.am b0cb6cc
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/utils.hpp caf5926
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/path.hpp 80d9bc6
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 108
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line108>
> >
> > Instead of opening the file here, can we have a new API os::access in stout which wraps the libc function "access"?
+1
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10375
-----------------------------------------------------------
On Aug. 15, 2012, 2:48 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2012, 2:48 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am b0cb6cc
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp caf5926
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/path.hpp 80d9bc6
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.hpp, line 26
> > <https://reviews.apache.org/r/6617/diff/1/?file=140337#file140337line26>
> >
> > newline if two includes do not share the same prefix.
done
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 439
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line439>
> >
> > Yes, i think you need to delete the 'process' as it is spawned without setting manage=true.
ok done, I don't see why it has to be allocated with new in the first place?
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/tests/utils.hpp, line 89
> > <https://reviews.apache.org/r/6617/diff/1/?file=140341#file140341line89>
> >
> > Can we just use mkdir here? The new name looks a little weird to me:)
done
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 230
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line230>
> >
> > Is there any other way to check whether the 'name' is a directory or not? This is not quite elegant to me:)
I'm taking this out but to be clear, this isn't checking if it's a directory, rather just whether it looks like a directory so that we can avoid unnecessarily resolving the name.
The actual directory check happens later using:
// Don't read directories.
if (os::exists(path.get(), true)) {
return NotFound("Cannot read a directory.");
}
Now though, with os::isdir
So since this / check is ugly and not necessary, I've taken it out.
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 108
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line108>
> >
> > Instead of opening the file here, can we have a new API os::access in stout which wraps the libc function "access"?
>
> Benjamin Hindman wrote:
> +1
good call! done.
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 205
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line205>
> >
> > We can get rid of it once we have the APIs above.
done
> On Aug. 16, 2012, 12:12 a.m., Jie Yu wrote:
> > src/files/files.cpp, line 194
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line194>
> >
> > Can we provide APIs in stout/os that basically have the same semantics as that in python APIs like os.path.isdir(path), os.path.isfile(path), etc.
done
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10375
-----------------------------------------------------------
On Aug. 17, 2012, 4:15 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2012, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am b0cb6cc
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp e783ac3
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp caf5926
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10375
-----------------------------------------------------------
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment22073>
newline if two includes do not share the same prefix.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22079>
Instead of opening the file here, can we have a new API os::access in stout which wraps the libc function "access"?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22083>
Can we provide APIs in stout/os that basically have the same semantics as that in python APIs like os.path.isdir(path), os.path.isfile(path), etc.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22084>
We can get rid of it once we have the APIs above.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22085>
Is there any other way to check whether the 'name' is a directory or not? This is not quite elegant to me:)
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22088>
Yes, i think you need to delete the 'process' as it is spawned without setting manage=true.
src/tests/utils.hpp
<https://reviews.apache.org/r/6617/#comment22089>
Can we just use mkdir here? The new name looks a little weird to me:)
- Jie Yu
On Aug. 15, 2012, 2:48 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2012, 2:48 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am b0cb6cc
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp caf5926
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/path.hpp 80d9bc6
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 24, 2012, 8:59 p.m., Vinod Kone wrote:
> > lgtm
thanks! I'll be republishing shortly with the get being asynchronous, I'll let you know.
> On Aug. 24, 2012, 8:59 p.m., Vinod Kone wrote:
> > src/master/slaves_manager.cpp, line 842
> > <https://reviews.apache.org/r/6617/diff/3-4/?file=143739#file143739line842>
> >
> > formatting
looks like these formatting issues were from some old tabs someone left in, fixed.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10748
-----------------------------------------------------------
On Aug. 23, 2012, 9:40 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2012, 9:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/slave.cpp 4efd41e
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10748
-----------------------------------------------------------
Ship it!
lgtm
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment23184>
formatting
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment23185>
formatting
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment23186>
formatting
- Vinod Kone
On Aug. 23, 2012, 9:40 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2012, 9:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/slave.cpp 4efd41e
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11049
-----------------------------------------------------------
as a note, this doesn't work well with local runs since we don't generate an ID for the filesprocess
I'll go over this with ben, and the solution might be to:
-generate an ID using ID::generate("files")
-export the files id through the master/slave state json endpoints (so that we know how to construct the urls for reading files)
-it would be nice though to have ID::generate behave this way: "files", "files(1)", "files(2)", so that urls look nice on non-local clusters, this would clean up slave ids as well
- Ben Mahler
On Aug. 25, 2012, 12:30 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2012, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/slave.cpp 4efd41e
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > Awesomeness. Just a few quick cleanups and I'll commit this. Thanks!
alright, merged against trunk (since I see some commits were made), required a manual fix in statistics.cpp so have a quick look at that file
would you have preferred I didn't do that merge so that you can see the incremental diff first?
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > src/local/main.cpp, line 111
> > <https://reviews.apache.org/r/6617/diff/9/?file=151414#file151414line111>
> >
> > Please put this newline back so it's consistent with the other files (or change all the other files and give me a good reason for doing so ;).
hm? I think you were still looking at diff 9 here instead of 10, local/main.cpp is unmodified against trunk
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/json.hpp, lines 177-179
> > <https://reviews.apache.org/r/6617/diff/9-10/?file=151441#file151441line177>
> >
> > s/val/value/g ;)
old habits die hard :)
I'll get there!
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 181
> > <https://reviews.apache.org/r/6617/diff/9-10/?file=151410#file151410line181>
> >
> > You shouldn't need to wrap with 'string(' since the second argument is a string already.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11219
-----------------------------------------------------------
On Sept. 9, 2012, 4:12 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2012, 4:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > Awesomeness. Just a few quick cleanups and I'll commit this. Thanks!
>
> Ben Mahler wrote:
> alright, merged against trunk (since I see some commits were made), required a manual fix in statistics.cpp so have a quick look at that file
>
> would you have preferred I didn't do that merge so that you can see the incremental diff first?
>
> Benjamin Hindman wrote:
> Yes, I'd prefer to see the incremental diff. But more importantly, I'll be downloading your diff, and it shouldn't include changes that have already been committed. This is my usual work flow: (1) git checkout trunk (2) git pull (3) git checkout feature (4) git rebase trunk (5) make check (6) fix any issues (7) git diff trunk >feature.patch (8) reviewboard. Please update the diff so I can commit!
My workflow for the latest diff was exactly what you described. So it does not contain changes that have already been committed (incrementally it does -- as in diff 9 vs 10 would -- but the overall diff / patch file is against the latest trunk). Is that ok?
Is that workflow what you do _each_ time you're updating the diff with reviewboard?
If that is case you won't be getting nice incremental diffs if commits to trunk were made, right?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11219
-----------------------------------------------------------
On Sept. 10, 2012, 12:27 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2012, 12:27 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 767f97a
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 6fd0df0
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp c1a1493
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp c639031
> src/master/slaves_manager.cpp d14bccb
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp 3e2a8d5
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 317a96c
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp bb7416c
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp acccb80
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 8f4d9e7
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am 9e23024
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 51dea77
> third_party/libprocess/src/statistics.cpp 6916336
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 9, 2012, 10:41 p.m., Benjamin Hindman wrote:
> > Awesomeness. Just a few quick cleanups and I'll commit this. Thanks!
>
> Ben Mahler wrote:
> alright, merged against trunk (since I see some commits were made), required a manual fix in statistics.cpp so have a quick look at that file
>
> would you have preferred I didn't do that merge so that you can see the incremental diff first?
Yes, I'd prefer to see the incremental diff. But more importantly, I'll be downloading your diff, and it shouldn't include changes that have already been committed. This is my usual work flow: (1) git checkout trunk (2) git pull (3) git checkout feature (4) git rebase trunk (5) make check (6) fix any issues (7) git diff trunk >feature.patch (8) reviewboard. Please update the diff so I can commit!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11219
-----------------------------------------------------------
On Sept. 10, 2012, 12:27 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2012, 12:27 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 767f97a
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 6fd0df0
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp c1a1493
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp c639031
> src/master/slaves_manager.cpp d14bccb
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp 3e2a8d5
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 317a96c
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp bb7416c
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp acccb80
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 8f4d9e7
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am 9e23024
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 51dea77
> third_party/libprocess/src/statistics.cpp 6916336
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11219
-----------------------------------------------------------
Ship it!
Awesomeness. Just a few quick cleanups and I'll commit this. Thanks!
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24144>
You shouldn't need to wrap with 'string(' since the second argument is a string already.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24145>
Ditto above.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24146>
This one doesn't need c_str either (you removed it above).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24147>
Ditto last comment.
src/local/main.cpp
<https://reviews.apache.org/r/6617/#comment24148>
Please put this newline back so it's consistent with the other files (or change all the other files and give me a good reason for doing so ;).
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24149>
These are really cool!
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24150>
Does it make sense to put the "constant" first like in our other *_EQ or *_LT statements?
third_party/libprocess/include/stout/json.hpp
<https://reviews.apache.org/r/6617/#comment24151>
s/val/value/g ;)
- Benjamin Hindman
On Sept. 9, 2012, 4:12 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2012, 4:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 10, 2012, 12:27 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
merged against trunk ..and trunk is currently broken on linux, but not my fault this time! ;)
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am 767f97a
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 6fd0df0
src/local/local.cpp 9c1c5b6
src/logging/logging.cpp c1a1493
src/master/http.cpp c480bc6
src/master/main.cpp 48859f1
src/master/master.hpp 866f3ef
src/master/master.cpp c639031
src/master/slaves_manager.cpp d14bccb
src/slave/http.cpp a1f7926
src/slave/main.cpp f1aade2
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp 3e2a8d5
src/tests/allocator_tests.cpp 80ba39d
src/tests/allocator_zookeeper_tests.cpp c4956dc
src/tests/configurator_tests.cpp c2f5aa0
src/tests/fault_tolerance_tests.cpp 317a96c
src/tests/files_tests.cpp PRE-CREATION
src/tests/gc_tests.cpp bb7416c
src/tests/master_detector_tests.cpp 5b2ab4f
src/tests/master_tests.cpp acccb80
src/tests/resource_offers_tests.cpp 1d7d9ed
src/tests/stout_tests.cpp 8f4d9e7
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am 9e23024
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/process/nothing.hpp c11a010
third_party/libprocess/include/process/once.hpp 46be55c
third_party/libprocess/include/process/pid.hpp 6a2eec9
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
third_party/libprocess/include/stout/numify.hpp ffd83ae
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 51dea77
third_party/libprocess/src/statistics.cpp 6916336
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 9, 2012, 4:12 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
diff from benh's review
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am 19bd20d
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 9cedea5
src/local/local.cpp 9c1c5b6
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/main.cpp 48859f1
src/master/master.hpp 866f3ef
src/master/master.cpp ab516ec
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/main.cpp f1aade2
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp ced232d
src/tests/allocator_tests.cpp 80ba39d
src/tests/allocator_zookeeper_tests.cpp c4956dc
src/tests/configurator_tests.cpp c2f5aa0
src/tests/fault_tolerance_tests.cpp 0e88701
src/tests/files_tests.cpp PRE-CREATION
src/tests/gc_tests.cpp 66bdbb2
src/tests/master_detector_tests.cpp 5b2ab4f
src/tests/master_tests.cpp c77a4ed
src/tests/resource_offers_tests.cpp 1d7d9ed
src/tests/stout_tests.cpp 0bc60a9
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am f898469
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/process/nothing.hpp c11a010
third_party/libprocess/include/process/once.hpp 46be55c
third_party/libprocess/include/process/pid.hpp 6a2eec9
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
third_party/libprocess/include/stout/numify.hpp ffd83ae
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.hpp, lines 60-62
> > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line60>
> >
> > s/upid/pid/g
> >
> > In the future, we'll eliminate UPID and just use PID<> when we don't know the type and PID<T> when we do.
>
> Ben Mahler wrote:
> in this case I used PID<> since I don't want to expose FilesProcess, just double checking that's what you wanted?
Yup!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3144
> > <https://reviews.apache.org/r/6617/diff/9/?file=151448#file151448line3144>
> >
> > Please add the following at the end of this line:
> >
> > // TODO(benh): Remove 'const &' after fixing libprocess.
>
> Ben Mahler wrote:
> done, lost me on this one..?
Normally you wouldn't need to make a primitive like size_t be const &, but in this case I only have Future::then versions for arguments that are const &, so that's why it has to be const &. This is a bug on my side.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11205
-----------------------------------------------------------
On Sept. 9, 2012, 4:12 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2012, 4:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 186
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line186>
> >
> > Don't need to invoke .c_str()! Also, love to hear your thoughts on eliminating the Try return type of strings::format ... read: I'm not a fan of calling .get() on a Try without checking first.
>
> Ben Mahler wrote:
> neither am I
>
> is this the *only* reason format can fail:
> return Try<std::string>::error(
> "Failed to format '" + fmt + "' (possibly out of memory)");
>
> if so, I don't see why we can't kill that, given we'd die from OOM anyway?
Well, we'd probably just want to kill ourselves (we might not actually get killed from OOM). We should clean this up in a separate review.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11205
-----------------------------------------------------------
On Sept. 9, 2012, 4:12 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2012, 4:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/process/nothing.hpp c11a010
> third_party/libprocess/include/process/once.hpp 46be55c
> third_party/libprocess/include/process/pid.hpp 6a2eec9
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/nothing.hpp PRE-CREATION
> third_party/libprocess/include/stout/numify.hpp ffd83ae
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> >
thanks for the thorough review!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.hpp, line 49
> > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line49>
> >
> > How come you made this virtual? Do you plan on extending Files?
reverted
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.hpp, lines 60-62
> > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line60>
> >
> > s/upid/pid/g
> >
> > In the future, we'll eliminate UPID and just use PID<> when we don't know the type and PID<T> when we do.
in this case I used PID<> since I don't want to expose FilesProcess, just double checking that's what you wanted?
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.hpp, line 69
> > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line69>
> >
> > s/the/our/
> > s/Repre/repre/
> >
> > Also, this is more the "file status" representation (i.e., it doesn't include the actual file content/bytes), so perhaps a more descriptive name?
changed to jsonFileInfo
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 59
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line59>
> >
> > What's your thinking for making the HTTP endpoint functions public?
artifact of pre http::get code, reverted
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.hpp, line 76
> > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line76>
> >
> > Is this really necessary? Can't one just get this via: basename(path)?
> >
> > In which case, you can simplify this function to just take one 'string path' argument instead of 'string dir' and 'string name'.
> >
> > Plus, 'name' is starting to get confusing to me ... i.e., is this the mapped 'name' from Files::attach?
Did some renaming around name to instead use:
path -> parameter
resolvedPath -> actual fs path
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 100
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line100>
> >
> > Exposing R_OK, W_OK, and X_OK might be preferred since then you know at the call site what's being checked (without having to go look at the declaration of os::access), e.g.:
> >
> > os::access(result.get(), R_OK);
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 129
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line129>
> >
> > I'd prefer to keep the variable names equal to the actual query key, so s/name/path/.
so along with some other changes, I removed name in favor of path (and resolvedPath for the actual fs path)
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 136
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line136>
> >
> > Then this can be s/path/resolvedPath/ or something of the sort.
heh, already done!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 146
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line146>
> >
> > Any reason not to use a JSON::Array to start? Are you concerned about duplicates? I don't think you can get duplicates. Or are you using the map to get sort ordering? Why is the sort ordering important?
the latter
so this initially used just the array, but when I went and tested on CentOS, the output ordering was different, so the sort is for my tests
unfortunately we don't have string -> JSON or JSON equality so this was the best I could do atm, I had made a ticket for this:
https://issues.apache.org/jira/browse/MESOS-269
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 152
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line152>
> >
> > Since PLOG will print the "reason" at the end, let's embed the file name in the message, i.e.,
> >
> > PLOG(WARNING) << "Found " << fullPath << " in ls but stat failed";
> >
> > Also, let's use full path (more info the better for log lines).
already done!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 156
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line156>
> >
> > I think having os::isdir is great, but you might as well not do the extra stat call here and use S_ISDIR (everywhere else it makes sense to just do os::isdir though).
was originally doing that, so reverted :)
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 186
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line186>
> >
> > Don't need to invoke .c_str()! Also, love to hear your thoughts on eliminating the Try return type of strings::format ... read: I'm not a fan of calling .get() on a Try without checking first.
neither am I
is this the *only* reason format can fail:
return Try<std::string>::error(
"Failed to format '" + fmt + "' (possibly out of memory)");
if so, I don't see why we can't kill that, given we'd die from OOM anyway?
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 188
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line188>
> >
> > Yes, we should totally inlude the error, but this should probably be a BadRequest.
wow I was all over the place with these!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 200
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line200>
> >
> > I think the numify error output is actually sufficient here, so you can just do (here and above):
> >
> > return BadRequest("Failed to numify 'length': " + result.error());
> >
> > Or if you want to log it too:
> >
> > string error = "Failed to numify 'length': " + result.error();
> > LOG(ERROR) << error;
> > return BadRequest(error);
> >
> > Also, I had put numify in quotes before, not sure if you want to do that or not (or use a different word like parse)? I thought numify was a word I made up. ;)
much nicer!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 327
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line327>
> >
> > Oh come on, 'key' and 'value' aren't that much harder to type than k and v! ;) That or 'name' and 'path' since that maps well to the attach nomenclature.
Devil's advocate: Oh come on, "index" isn't that much harder to type than i! ;)
k and v are like i and j IMO (I bet we disagree here), but changed to name and path since I like that better!
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 335
> > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line335>
> >
> > Can you include a comment explaining what a "some" versus "none" versus "error" result means?
done, added above where it's prototype is
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 533
> > <https://reviews.apache.org/r/6617/diff/9/?file=151419#file151419line533>
> >
> > You can actually pull this to the top of the function as a CHECK(!result.isDiscarded);
ok I didn't really know the semantics of being discarded, and didn't expect that to be CHECK worthy (killing the program)
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/master/slaves_manager.cpp, line 816
> > <https://reviews.apache.org/r/6617/diff/9/?file=151420#file151420line816>
> >
> > If we had a version of numify like so:
> >
> > -----------------
> > template <typename T>
> > Result<T> numify(const Option<std::string>& option)
> > {
> > if (option.isSome()) {
> > return numify(option.get()); // Assumes we add a Result(const Try& t) constructor.
> > }
> > return Result<T>::none();
> > }
> > -----------------
> >
> > Then we could make this code even simpler:
> >
> > -----------------
> > Option<string> hostname = request.query.get("hostname");
> > Result<uint16_t> port = numify<uint16_t>(request.query.get("port"));
> >
> > if (hostname.isNone()) {
> > ...;
> > }
> >
> > if (port.isNone()) {
> > ...;
> > } else if (port.isError()) {
> > ...;
> > }
> >
> > ... hostname.get() ... port.get() ...;
> > -----------------
> >
> > You don't have to worry about this now if you don't want ...
I'm gonna punt on even more refactoring in this CL, added a TODO
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, line 89
> > <https://reviews.apache.org/r/6617/diff/9/?file=151442#file151442line89>
> >
> > See comment at call site.
fixed
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, lines 3197-3198
> > <https://reviews.apache.org/r/6617/diff/9/?file=151448#file151448line3197>
> >
> > If the function + args fits on _newline_, please keep them together.
I might be guilty of this in other places in this review, aware of this now :)
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3144
> > <https://reviews.apache.org/r/6617/diff/9/?file=151448#file151448line3144>
> >
> > Please add the following at the end of this line:
> >
> > // TODO(benh): Remove 'const &' after fixing libprocess.
done, lost me on this one..?
> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote:
> > src/tests/files_tests.cpp, line 76
> > <https://reviews.apache.org/r/6617/diff/9/?file=151429#file151429line76>
> >
> > Okay, these last three functions can be generally useful as macros.
> >
> > For the first one:
> >
> > ASSERT_FUTURE_WILL_SUCCEED
> > EXPECT_FUTURE_WILL_SUCCEED
> >
> > Note that I use "will" to capture that this is doing an await as well. You could also imagine:
> >
> > *_FUTURE_WILL_READY
> >
> > Or:
> >
> > *_FUTURE_AWAIT_READY
> >
> > Or something similar.
> >
> > For the second one:
> >
> > ASSERT_FUTURE_WILL_FAIL
> > EXPECT_FUTURE_WILL_FAIL
> >
> > For the third one:
> >
> > ASSERT_FUTURE_WILL_EQ
> > EXPECT_FUTURE_WILL_EQ
> >
> > Then for any that are specific to a type (e.g., checkStatus), I still think it would be great to see shareable macros along the lines of:
> >
> > ASSERT_RESPONSE_STATUS_WILL_EQ(OK().status, process::http::get(...));
> >
> > You can stick these in src/tests/utils.hpp. There are lots of other tests that will be able to take advantage of these. That will be awesome!
done, but I made them functions rather than macros, since I operate on the future multiple times (and that doesn't play well with macros)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11205
-----------------------------------------------------------
On Sept. 7, 2012, 10:49 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2012, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/local/main.cpp ecab6b8
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11205
-----------------------------------------------------------
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment24002>
How come you made this virtual? Do you plan on extending Files?
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment24003>
Can you move nothing.hpp into stout please?
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment24005>
s/upid/pid/g
In the future, we'll eliminate UPID and just use PID<> when we don't know the type and PID<T> when we do.
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment24014>
s/the/our/
s/Repre/repre/
Also, this is more the "file status" representation (i.e., it doesn't include the actual file content/bytes), so perhaps a more descriptive name?
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment24015>
Is this really necessary? Can't one just get this via: basename(path)?
In which case, you can simplify this function to just take one 'string path' argument instead of 'string dir' and 'string name'.
Plus, 'name' is starting to get confusing to me ... i.e., is this the mapped 'name' from Files::attach?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24016>
What's your thinking for making the HTTP endpoint functions public?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24017>
Exposing R_OK, W_OK, and X_OK might be preferred since then you know at the call site what's being checked (without having to go look at the declaration of os::access), e.g.:
os::access(result.get(), R_OK);
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24018>
I generally really like using the 'verb' name (or some variant of it) as the variable name, e.g., access from os::access (or accessible from os::access). Makes for really readable code! Since we stick all of our functions behind namespaces (like 'os'), this is usually always possible. Yeah!
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24020>
I'd prefer to keep the variable names equal to the actual query key, so s/name/path/.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24019>
s/directory/path/
Also, this comment is pretty redundant given the readability of the next line.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24021>
Then this can be s/path/resolvedPath/ or something of the sort.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24022>
Any reason not to use a JSON::Array to start? Are you concerned about duplicates? I don't think you can get duplicates. Or are you using the map to get sort ordering? Why is the sort ordering important?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24012>
Since PLOG will print the "reason" at the end, let's embed the file name in the message, i.e.,
PLOG(WARNING) << "Found " << fullPath << " in ls but stat failed";
Also, let's use full path (more info the better for log lines).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24023>
I think having os::isdir is great, but you might as well not do the extra stat call here and use S_ISDIR (everywhere else it makes sense to just do os::isdir though).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24024>
Ditto as comment above.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24025>
Ditto above.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24026>
BadRequest?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24027>
One of the nice things about exposing the 'query' hashmap like you did means you can write stuff like this:
if (request.query.get("offset").isSome()) {
Try<off_t> result = numify<off_t>(request.query.get("offset").get());
...;
offset = result.get();
}
I tend to like that style better because then you don't have to introduce as many temporary variables.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24029>
Don't need to invoke .c_str()! Also, love to hear your thoughts on eliminating the Try return type of strings::format ... read: I'm not a fan of calling .get() on a Try without checking first.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24028>
Yes, we should totally inlude the error, but this should probably be a BadRequest.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24030>
I think the numify error output is actually sufficient here, so you can just do (here and above):
return BadRequest("Failed to numify 'length': " + result.error());
Or if you want to log it too:
string error = "Failed to numify 'length': " + result.error();
LOG(ERROR) << error;
return BadRequest(error);
Also, I had put numify in quotes before, not sure if you want to do that or not (or use a different word like parse)? I thought numify was a word I made up. ;)
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24031>
Is this an internal server error or a bad request?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24034>
Again, shouldn't need c_str.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24035>
Add a TODO to do this via the async process::read please.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24036>
Oh come on, 'key' and 'value' aren't that much harder to type than k and v! ;) That or 'name' and 'path' since that maps well to the attach nomenclature.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24037>
Can you include a comment explaining what a "some" versus "none" versus "error" result means?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24038>
I'd move this just above the while loop (it's kind of hidden now).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24041>
See comment below about chroot, possibly changing wording here too.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24039>
if (!strings::startsWith(result.get(), paths[prefix])) {
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment24040>
Maybe "Resolved path is inaccessible" rather than "breaks out of chroot", since the latter is a bit more esoteric to understand in this circumstance.
src/local/main.cpp
<https://reviews.apache.org/r/6617/#comment24042>
Do the other main.cpp files not have a newline in between these calls?
src/logging/logging.cpp
<https://reviews.apache.org/r/6617/#comment24043>
Brilliant!
src/master/http.cpp
<https://reviews.apache.org/r/6617/#comment24044>
Any reason not to jsonp it?
src/master/master.hpp
<https://reviews.apache.org/r/6617/#comment24045>
const &
src/master/master.cpp
<https://reviews.apache.org/r/6617/#comment24046>
You can actually pull this to the top of the function as a CHECK(!result.isDiscarded);
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment24047>
If we had a version of numify like so:
-----------------
template <typename T>
Result<T> numify(const Option<std::string>& option)
{
if (option.isSome()) {
return numify(option.get()); // Assumes we add a Result(const Try& t) constructor.
}
return Result<T>::none();
}
-----------------
Then we could make this code even simpler:
-----------------
Option<string> hostname = request.query.get("hostname");
Result<uint16_t> port = numify<uint16_t>(request.query.get("port"));
if (hostname.isNone()) {
...;
}
if (port.isNone()) {
...;
} else if (port.isError()) {
...;
}
... hostname.get() ... port.get() ...;
-----------------
You don't have to worry about this now if you don't want ...
src/slave/http.cpp
<https://reviews.apache.org/r/6617/#comment24048>
Pass jsonp?
src/slave/slave.hpp
<https://reviews.apache.org/r/6617/#comment24049>
const &
src/slave/slave.cpp
<https://reviews.apache.org/r/6617/#comment24050>
Same comment about CHECK as in master.cpp.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24051>
Please add to json.hpp an operator << (ostream, ...) overload and then use stringify everywhere you call jstonToString. Thanks!
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24052>
const &
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24053>
Ditto.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24054>
Ditto.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24055>
Okay, these last three functions can be generally useful as macros.
For the first one:
ASSERT_FUTURE_WILL_SUCCEED
EXPECT_FUTURE_WILL_SUCCEED
Note that I use "will" to capture that this is doing an await as well. You could also imagine:
*_FUTURE_WILL_READY
Or:
*_FUTURE_AWAIT_READY
Or something similar.
For the second one:
ASSERT_FUTURE_WILL_FAIL
EXPECT_FUTURE_WILL_FAIL
For the third one:
ASSERT_FUTURE_WILL_EQ
EXPECT_FUTURE_WILL_EQ
Then for any that are specific to a type (e.g., checkStatus), I still think it would be great to see shareable macros along the lines of:
ASSERT_RESPONSE_STATUS_WILL_EQ(OK().status, process::http::get(...));
You can stick these in src/tests/utils.hpp. There are lots of other tests that will be able to take advantage of these. That will be awesome!
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24056>
s/upid/pid/
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment24057>
s/upid/pid/
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment24058>
Stick this inside the "query" namespace and just call the function "parse" please.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment24059>
After doing above, this will probably all fit on one line.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment24061>
s/upid/pid/
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment24071>
&
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment24060>
'{' on newline please.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment24062>
See comment at call site.
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment24063>
Please add TODO: Make DataDecoder abstract (pure-virtual) class and make RequestDecoder be a concrete subclass.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24064>
YES!
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24065>
Awesome!
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24067>
Please add the following at the end of this line:
// TODO(benh): Remove 'const &' after fixing libprocess.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24068>
If the function + args fits on _newline_, please keep them together.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24069>
s/copy/response/
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24070>
s/upid/pid/ and const & on string parameters.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24072>
;)
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment24073>
Unused?
third_party/libprocess/src/statistics.cpp
<https://reviews.apache.org/r/6617/#comment24074>
Another place to use numify(option)!
- Benjamin Hindman
On Sept. 7, 2012, 10:49 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2012, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/local/local.cpp 9c1c5b6
> src/local/main.cpp ecab6b8
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/main.cpp 48859f1
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/main.cpp f1aade2
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/allocator_tests.cpp 80ba39d
> src/tests/allocator_zookeeper_tests.cpp c4956dc
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/fault_tolerance_tests.cpp 0e88701
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/gc_tests.cpp 66bdbb2
> src/tests/master_detector_tests.cpp 5b2ab4f
> src/tests/master_tests.cpp c77a4ed
> src/tests/resource_offers_tests.cpp 1d7d9ed
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 7, 2012, 10:49 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
Updated with benh's comments:
-Local runs use global Files object
-http::get() uses Future::failed() in favor of PLOG(FATAL)
Tested on Lion and CentOS
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am 19bd20d
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 9cedea5
src/local/local.cpp 9c1c5b6
src/local/main.cpp ecab6b8
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/main.cpp 48859f1
src/master/master.hpp 866f3ef
src/master/master.cpp ab516ec
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/main.cpp f1aade2
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp ced232d
src/tests/allocator_tests.cpp 80ba39d
src/tests/allocator_zookeeper_tests.cpp c4956dc
src/tests/configurator_tests.cpp c2f5aa0
src/tests/fault_tolerance_tests.cpp 0e88701
src/tests/files_tests.cpp PRE-CREATION
src/tests/gc_tests.cpp 66bdbb2
src/tests/master_detector_tests.cpp 5b2ab4f
src/tests/master_tests.cpp c77a4ed
src/tests/resource_offers_tests.cpp 1d7d9ed
src/tests/stout_tests.cpp 0bc60a9
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am f898469
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 7, 2012, 12:15 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
This now uses a single FilesProcess per libprocess process.
This means that local runs can work properly when there are masters and slaves within the same process.
Fixed a broken test on linux due to 'ls' output ordering.
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
include/mesos/scheduler.hpp ed30884
src/Makefile.am 19bd20d
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 9cedea5
src/local/local.hpp 96a1206
src/local/local.cpp 9c1c5b6
src/local/main.cpp ecab6b8
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/main.cpp 48859f1
src/master/master.hpp 866f3ef
src/master/master.cpp ab516ec
src/master/slaves_manager.cpp e25efd0
src/sched/sched.cpp 6267fab
src/slave/http.cpp a1f7926
src/slave/main.cpp f1aade2
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp ced232d
src/tests/allocator_tests.cpp 80ba39d
src/tests/allocator_zookeeper_tests.cpp c4956dc
src/tests/configurator_tests.cpp c2f5aa0
src/tests/exception_tests.cpp bb3f2c2
src/tests/fault_tolerance_tests.cpp 0e88701
src/tests/files_tests.cpp PRE-CREATION
src/tests/gc_tests.cpp 66bdbb2
src/tests/master_detector_tests.cpp 5b2ab4f
src/tests/master_tests.cpp c77a4ed
src/tests/resource_offers_tests.cpp 1d7d9ed
src/tests/stout_tests.cpp 0bc60a9
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am f898469
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 6, 2012, 12:14 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
vinod's review
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am 19bd20d
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 9cedea5
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/master.hpp 866f3ef
src/master/master.cpp ab516ec
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp ced232d
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/stout_tests.cpp 0bc60a9
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am f898469
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 5, 2012, 11:52 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 310
> > <https://reviews.apache.org/r/6617/diff/4-6/?file=145099#file145099line310>
> >
> > s/slsave/slave/
nice catch, thanks
> On Sept. 5, 2012, 11:52 p.m., Vinod Kone wrote:
> > src/files/files.hpp, line 53
> > <https://reviews.apache.org/r/6617/diff/4-6/?file=145093#file145093line53>
> >
> > woah.. Nothing! sweet. How hard is it to actually extend Future to be parametrized by void.
> > I think Future<void> would be awesome to have.
Has to be done by template specialization, and based on the size of future.hpp, I think we can agree against it ;)
Benh and I were discussing this though, and I think he preferred making a Void type to replace Nothing. I think I prefer that as well but hesitant to refactor even more code in this CL.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11075
-----------------------------------------------------------
On Sept. 6, 2012, 12:14 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2012, 12:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review11075
-----------------------------------------------------------
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment23777>
woah.. Nothing! sweet. How hard is it to actually extend Future to be parametrized by void.
I think Future<void> would be awesome to have.
src/slave/slave.cpp
<https://reviews.apache.org/r/6617/#comment23779>
s/slsave/slave/
src/tests/stout_tests.cpp
<https://reviews.apache.org/r/6617/#comment23781>
doesn't this test make the above 2 obsolete?
third_party/libprocess/include/stout/strings.hpp
<https://reviews.apache.org/r/6617/#comment23787>
unused
third_party/libprocess/include/stout/strings.hpp
<https://reviews.apache.org/r/6617/#comment23786>
s/, empty/. Empty
third_party/libprocess/include/stout/strings.hpp
<https://reviews.apache.org/r/6617/#comment23785>
s/Empties/Empty
- Vinod Kone
On Sept. 5, 2012, 10:30 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2012, 10:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am 19bd20d
> src/common/attributes.cpp 66d0b70
> src/common/resources.cpp 2cee743
> src/common/values.cpp ec341be
> src/configurator/configuration.hpp e2cd1b5
> src/configurator/configurator.cpp 3916427
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/launcher/main.cpp 06597e6
> src/linux/cgroups.cpp 9cedea5
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/master.hpp 866f3ef
> src/master/master.cpp ab516ec
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/slave/slave.hpp b7ab2ab
> src/slave/slave.cpp ced232d
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/stout_tests.cpp 0bc60a9
> src/tests/utils.hpp 54da799
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/Makefile.am f898469
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp df0f7ff
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/include/stout/strings.hpp 0646bf9
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp 55b5d50
> third_party/libprocess/src/process.cpp 2b2d521
> third_party/libprocess/src/statistics.cpp d05b327
> third_party/libprocess/src/tokenize.hpp f886186
> third_party/libprocess/src/tokenize.cpp 759ce5f
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Sept. 5, 2012, 10:30 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
Several more changes here..
Files:
-Changed attach from Future<bool> to Future<Nothing>
-Better error messages / responses from file http endpoints
-Debug http endpoint for files to introspect on the attached paths
-Master / Slave now check result of the attach (using defer)
Misc:
-Deleted libprocess/src/tokenize.{hpp,cpp} and moved tokenize into stout/strings.hpp
-strings::split did not allow empties! (caused a bug in file resolution) Fixed this and adjusted all callers relying on empties to use tokenize() instead.
-Adjusted the tests for tokenize() and split() in stout_tests.cpp
This change is getting gigantic.. may have to sit down and pair with benh to get this in.
NOTE: This does not fix the issues on local runs (master/slave hardcode the log filename, on local it's different: mesos-master.INFO -> mesos-local.INFO)
Glog does not allow retrieving the filename: http://code.google.com/p/google-glog/issues/detail?id=116
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am 19bd20d
src/common/attributes.cpp 66d0b70
src/common/resources.cpp 2cee743
src/common/values.cpp ec341be
src/configurator/configuration.hpp e2cd1b5
src/configurator/configurator.cpp 3916427
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/launcher/main.cpp 06597e6
src/linux/cgroups.cpp 9cedea5
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/master.hpp 866f3ef
src/master/master.cpp ab516ec
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/slave.hpp b7ab2ab
src/slave/slave.cpp ced232d
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/stout_tests.cpp 0bc60a9
src/tests/utils.hpp 54da799
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/Makefile.am f898469
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp df0f7ff
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/include/stout/strings.hpp 0646bf9
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp 55b5d50
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
third_party/libprocess/src/tokenize.hpp f886186
third_party/libprocess/src/tokenize.cpp 759ce5f
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Aug. 25, 2012, 12:30 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
Now with async read of the http response (thanks for the help benh!)
Please review:
new io::read() for buffered data (io.hpp, process.cpp)
http::get() implementation (process.cpp)
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am aaceee3
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/slave.cpp 4efd41e
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/utils.hpp 83d5daa
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp b1eceb3
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp ac4886e
third_party/libprocess/src/process.cpp 2b2d521
third_party/libprocess/src/statistics.cpp d05b327
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Aug. 23, 2012, 9:40 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
updated diff
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am aaceee3
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/slave/slave.cpp 4efd41e
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/utils.hpp 83d5daa
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp b1eceb3
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp ac4886e
third_party/libprocess/src/process.cpp 78069bf
third_party/libprocess/src/statistics.cpp d05b327
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> >
>
> Jie Yu wrote:
> Ben, I don't quite understand get you 'TODO' in io::read. Why can't we reuse that for http::get()?
io::read performs a single non-blocking read on the socket into an external buffer of fixed size
with get() I need to read a response of unknown size, until I get a completed HttpResponse or the socket is closed, etc.
this means in order to re-use io::read I will either have to write my own variant (i.e. not re-use it), or try to loop through io::read with futures.. I had trouble wiring the latter up =/
But I'll probably have another go at it today now that my brain is fresh :)
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> > third_party/libprocess/src/process.cpp, line 3175
> > <https://reviews.apache.org/r/6617/diff/3/?file=143754#file143754line3175>
> >
> > This function actually blocks on recv(). This will block a libprocess worker thread. That makes me feel that we should never use this function except in tests.
yes I know, as I said in the publish comments, this first is a blocking get, and I'll be re-publishing with a non-blocking get afterwards, this review was mostly for the rest of the code
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> > third_party/libprocess/src/process.cpp, line 3179
> > <https://reviews.apache.org/r/6617/diff/3/?file=143754#file143754line3179>
> >
> > Since you pass flags '0' to recv, I don't think you can get these two errno.
> >
> > You should specify MSG_DONTWAIT when calling recv().
> >
> > Also, see my above comment.
alright, I guess that's fine, but it's redundant since I have O_NONBLOCK (I've commented that part out, since this was the blocking version for now).
"Enables nonblocking operation; if the operation would block, the call fails with the error EAGAIN or EWOULDBLOCK (this can also be enabled using the O_NONBLOCK flag with the F_SETFL fcntl(2))."
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10605
-----------------------------------------------------------
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> >
>
> Jie Yu wrote:
> Ben, I don't quite understand get you 'TODO' in io::read. Why can't we reuse that for http::get()?
>
> Ben Mahler wrote:
> io::read performs a single non-blocking read on the socket into an external buffer of fixed size
>
> with get() I need to read a response of unknown size, until I get a completed HttpResponse or the socket is closed, etc.
> this means in order to re-use io::read I will either have to write my own variant (i.e. not re-use it), or try to loop through io::read with futures.. I had trouble wiring the latter up =/
>
> But I'll probably have another go at it today now that my brain is fresh :)
Yeah, I think you might need to put this logic into a Process. io::read is like a primitive as far as I can understand.
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> > third_party/libprocess/src/process.cpp, line 3179
> > <https://reviews.apache.org/r/6617/diff/3/?file=143754#file143754line3179>
> >
> > Since you pass flags '0' to recv, I don't think you can get these two errno.
> >
> > You should specify MSG_DONTWAIT when calling recv().
> >
> > Also, see my above comment.
>
> Ben Mahler wrote:
> alright, I guess that's fine, but it's redundant since I have O_NONBLOCK (I've commented that part out, since this was the blocking version for now).
>
> "Enables nonblocking operation; if the operation would block, the call fails with the error EAGAIN or EWOULDBLOCK (this can also be enabled using the O_NONBLOCK flag with the F_SETFL fcntl(2))."
aha, I didn't notice your comment:) Yeah, setting O_NONBLOCK is fine.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10605
-----------------------------------------------------------
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 22, 2012, 2:18 a.m., Jie Yu wrote:
> >
Ben, I don't quite understand get you 'TODO' in io::read. Why can't we reuse that for http::get()?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10605
-----------------------------------------------------------
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10605
-----------------------------------------------------------
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment22694>
This function actually blocks on recv(). This will block a libprocess worker thread. That makes me feel that we should never use this function except in tests.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment22695>
Since you pass flags '0' to recv, I don't think you can get these two errno.
You should specify MSG_DONTWAIT when calling recv().
Also, see my above comment.
- Jie Yu
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/http.hpp, line 151
> > <https://reviews.apache.org/r/6617/diff/3/?file=143745#file143745line151>
> >
> > I'd actually just like to see a "statuses" map, just like process/mime.hpp for mime types.
done similarly to mime now
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/http.hpp, lines 7-8
> > <https://reviews.apache.org/r/6617/diff/3/?file=143745#file143745line7>
> >
> > In libprocess we've actually included everything "explicitely", via <process/future.hpp> and <process/pid.hpp> see other files.
> >
> > Note this was not the case with stout, but we may change that in the future.
done
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, line 88
> > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line88>
> >
> > Any reason not to return a Try and capture an error string?
ok done, I return false on EACCES, and error otherwise
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/decoder.hpp, line 347
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line347>
> >
> > Is there a way to capture that this function is not expected to get called in the parser?
I suppose I could do an assert if you like?
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/decoder.hpp, line 386
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line386>
> >
> > This is a copy-paste thing, but please go through and remove everything that isn't needed.
whoops, just query isn't needed
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/decoder.hpp, line 82
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line82>
> >
> > Two suggestions:
> >
> > (1) Factor this out into the http::query namespace as a function called 'parse'. Never know if/when it might be useful to others, and to me it "fits" nicely there (very independent piece of code, needs a nice place to live).
> >
> > (2) Return the hashmap rather than take the pointer result and let the compiler do the optimization (eliding copies, etc). In the future we can be even more explicit with C++11 move semantics without having to change the structure (and in many cases syntax) of our code.
done and done
> On Aug. 23, 2012, 5:47 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, lines 333-339
> > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line333>
> >
> > Also, we already have 'exists', which takes a bool as a second argument to check whether or not it's a directory. I'm not convinced 'exists' is better than isdir and isfile, but we should be opinionated about how we want to do things, and remove the exists mechanism (at least the checking directory part) if we decide not to use it.
ok so I've removed the directory flag in exists (which I thought made exist calls a bit confusing to the unenlightened reader)
now, exists is effectively (isfile() || isdir), I cleaned up the directory flag using calls, but I left all other exists calls, shall I wipe those too? They seem to always know whether they want a file or a dir.. just want to double check before I do even more cleanup
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10666
-----------------------------------------------------------
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10666
-----------------------------------------------------------
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment22934>
In libprocess we've actually included everything "explicitely", via <process/future.hpp> and <process/pid.hpp> see other files.
Note this was not the case with stout, but we may change that in the future.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment22937>
Newline please, and a brief comment explaining what this function does.
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment22938>
const&
third_party/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/6617/#comment22935>
I'd actually just like to see a "statuses" map, just like process/mime.hpp for mime types.
third_party/libprocess/include/process/io.hpp
<https://reviews.apache.org/r/6617/#comment22933>
This isn't needed (yet).
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment22940>
Any reason not to return a Try and capture an error string?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment22941>
Also, we already have 'exists', which takes a bool as a second argument to check whether or not it's a directory. I'm not convinced 'exists' is better than isdir and isfile, but we should be opinionated about how we want to do things, and remove the exists mechanism (at least the checking directory part) if we decide not to use it.
third_party/libprocess/include/stout/stringify.hpp
<https://reviews.apache.org/r/6617/#comment22932>
Doesn't look like this is needed.
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22944>
Two suggestions:
(1) Factor this out into the http::query namespace as a function called 'parse'. Never know if/when it might be useful to others, and to me it "fits" nicely there (very independent piece of code, needs a nice place to live).
(2) Return the hashmap rather than take the pointer result and let the compiler do the optimization (eliding copies, etc). In the future we can be even more explicit with C++11 move semantics without having to change the structure (and in many cases syntax) of our code.
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22947>
Is there a way to capture that this function is not expected to get called in the parser?
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22946>
This is a copy-paste thing, but please go through and remove everything that isn't needed.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/6617/#comment22945>
We should be able to implement this directly in terms of the existing event loop. Let's pair on that together.
- Benjamin Hindman
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Mahler <bm...@twitter.com>.
only through vinod's review, benh's coming up next
On Thu, Aug 23, 2012 at 12:17 PM, Ben Mahler <be...@gmail.com>wrote:
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.hpp, line 59
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143735#file143735line59>
> > >
> > > i'm guessing u made this public to make it visible for testing? if
> yes, please add a comment saying so.
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, line 66
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line66>
> > >
> > > s/std::string/string
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, lines 77-78
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line77>
> > >
> > > Kill this
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, line 105
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line105>
> > >
> > > removing trailing "/" is a good utility function to have in strings
>
> seems succinct enough as it though using strings::remove
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, line 134
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line134>
> > >
> > > when does this happen?
>
> never :)
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, line 144
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line144>
> > >
> > > push this down to where its used
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/files/files.cpp, line 313
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line313>
> > >
> > > I dont' completely understand what's happening here. Can you add
> more comments/docs please?
>
> added an explanation with an example, hopefully it makes sense now?
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/master/slaves_manager.cpp, line 835
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line835>
> > >
> > > can't u use numify for this?
>
> yep, alright, more refactoring then
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/master/slaves_manager.cpp, line 921
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line921>
> > >
> > > ditto
>
> yep, done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, lines 19-21
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line19>
> > >
> > > reorder?
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 29
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line29>
> > >
> > > new line
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 56
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line56>
> > >
> > > for this test, why do u need something to the file?
>
> I don't really, removed
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, lines 107-108
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line107>
> > >
> > > are u supposed to test something here?
>
> nope, since it returns void, I'm just checking it doesn't crash
> I think this test will help against regressions though as well
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 132
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line132>
> > >
> > > s/4/strlen("body")/
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 151
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line151>
> > >
> > > why these blocks?
>
> so that I don't need to name things like request1, request2, ... I can
> just a nested namespaces instead
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 145
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line145>
> > >
> > > not needed?
>
> needed, mkdir is non-recursive
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 261
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line261>
> > >
> > > not necessary
>
> not recursive, so needed
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, line 179
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line179>
> > >
> > > s/5/strlen("three")/
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, lines 177-180
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line177>
> > >
> > > you should probably factor this out into a helper function.
>
> since I only do this twice, let's not bother
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/files_tests.cpp, lines 213-216
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line213>
> > >
> > > it would be factor this out to a helper too.
>
> done! this really trimmed down the test code :)
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/utils.hpp, line 92
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line92>
> > >
> > > i see why you were creating parent directories here. we already
> have this function in stout/os, which acts like 'mkdir-p'. use that instead.
>
> ah I see, killed my version
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > src/tests/utils.hpp, line 101
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line101>
> > >
> > > wait..dont we already have this abstraction in stout/os?
> > > if so, can you use that instead?
>
> ah yes, done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/include/stout/os.hpp, line 326
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line326>
> > >
> > > kill line
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/include/stout/os.hpp, lines 333-339
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line333>
> > >
> > > this seems like !isdir(), why do we need a different function?
>
> not quite.. !isDir does not imply isFile, and !isFile does not imply
> isDir, since the dir/file can be non existent
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/include/stout/os.hpp, line 338
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line338>
> > >
> > > kill line
>
> done
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/src/decoder.hpp, line 82
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line82>
> > >
> > > why static?
>
> ignoring, given benh's comments
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/src/decoder.hpp, line 303
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line303>
> > >
> > > need an assert(!decoder->failure) ?
>
> nope, just returning 0 now instead of pulling out the decoder
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/src/decoder.hpp, lines 345-363
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line345>
> > >
> > > add asserts for failure?
>
> well failure gets triggered in decode(), then on message begin the assert
> will check for failure, failure cannot be true in these functions
>
>
> > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > > third_party/libprocess/src/decoder.hpp, line 372
> > > <
> https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line372>
> > >
> > > new line
>
> done
>
>
> - Ben
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/#review10643
> -----------------------------------------------------------
>
>
> On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/6617/
> > -----------------------------------------------------------
> >
> > (Updated Aug. 22, 2012, 1:37 a.m.)
> >
> >
> > Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
> >
> >
> > Description
> > -------
> >
> > Implementing the file abstraction and http endpoints for file reading /
> browsing.
> >
> >
> > This addresses bug MESOS-255.
> > https://issues.apache.org/jira/browse/MESOS-255
> >
> >
> > Diffs
> > -----
> >
> > src/Makefile.am aaceee3
> > src/files/files.hpp d0cab91
> > src/files/files.cpp d4080d4
> > src/logging/logging.cpp 6909b0b
> > src/master/http.cpp c480bc6
> > src/master/slaves_manager.cpp e25efd0
> > src/slave/http.cpp a1f7926
> > src/tests/configurator_tests.cpp c2f5aa0
> > src/tests/files_tests.cpp PRE-CREATION
> > src/tests/utils.hpp 83d5daa
> > src/webui/master/static/controllers.js 1606e64
> > third_party/libprocess/include/process/http.hpp 8424ca6
> > third_party/libprocess/include/process/io.hpp 6a40b18
> > third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> > third_party/libprocess/include/stout/json.hpp 25dbcf4
> > third_party/libprocess/include/stout/os.hpp b1eceb3
> > third_party/libprocess/include/stout/path.hpp 80d9bc6
> > third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> > third_party/libprocess/src/decoder.hpp 105fe5d
> > third_party/libprocess/src/encoder.hpp ac4886e
> > third_party/libprocess/src/process.cpp 78069bf
> > third_party/libprocess/src/statistics.cpp d05b327
> >
> > Diff: https://reviews.apache.org/r/6617/diff/
> >
> >
> > Testing
> > -------
> >
> > Added files_tests.cpp
> > make check
> >
> >
> > Thanks,
> >
> > Ben Mahler
> >
> >
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.hpp, line 59
> > <https://reviews.apache.org/r/6617/diff/3/?file=143735#file143735line59>
> >
> > i'm guessing u made this public to make it visible for testing? if yes, please add a comment saying so.
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 66
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line66>
> >
> > s/std::string/string
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, lines 77-78
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line77>
> >
> > Kill this
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 105
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line105>
> >
> > removing trailing "/" is a good utility function to have in strings
seems succinct enough as it though using strings::remove
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 134
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line134>
> >
> > when does this happen?
never :)
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 144
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line144>
> >
> > push this down to where its used
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 313
> > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line313>
> >
> > I dont' completely understand what's happening here. Can you add more comments/docs please?
added an explanation with an example, hopefully it makes sense now?
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/master/slaves_manager.cpp, line 835
> > <https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line835>
> >
> > can't u use numify for this?
yep, alright, more refactoring then
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/master/slaves_manager.cpp, line 921
> > <https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line921>
> >
> > ditto
yep, done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, lines 19-21
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line19>
> >
> > reorder?
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 29
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line29>
> >
> > new line
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 56
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line56>
> >
> > for this test, why do u need something to the file?
I don't really, removed
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, lines 107-108
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line107>
> >
> > are u supposed to test something here?
nope, since it returns void, I'm just checking it doesn't crash
I think this test will help against regressions though as well
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 132
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line132>
> >
> > s/4/strlen("body")/
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 151
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line151>
> >
> > why these blocks?
so that I don't need to name things like request1, request2, ... I can just a nested namespaces instead
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 145
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line145>
> >
> > not needed?
needed, mkdir is non-recursive
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 261
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line261>
> >
> > not necessary
not recursive, so needed
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 179
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line179>
> >
> > s/5/strlen("three")/
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, lines 177-180
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line177>
> >
> > you should probably factor this out into a helper function.
since I only do this twice, let's not bother
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, lines 213-216
> > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line213>
> >
> > it would be factor this out to a helper too.
done! this really trimmed down the test code :)
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/utils.hpp, line 92
> > <https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line92>
> >
> > i see why you were creating parent directories here. we already have this function in stout/os, which acts like 'mkdir-p'. use that instead.
ah I see, killed my version
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > src/tests/utils.hpp, line 101
> > <https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line101>
> >
> > wait..dont we already have this abstraction in stout/os?
> > if so, can you use that instead?
ah yes, done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/os.hpp, line 326
> > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line326>
> >
> > kill line
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/os.hpp, lines 333-339
> > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line333>
> >
> > this seems like !isdir(), why do we need a different function?
not quite.. !isDir does not imply isFile, and !isFile does not imply isDir, since the dir/file can be non existent
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/os.hpp, line 338
> > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line338>
> >
> > kill line
done
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/decoder.hpp, line 82
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line82>
> >
> > why static?
ignoring, given benh's comments
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/decoder.hpp, line 303
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line303>
> >
> > need an assert(!decoder->failure) ?
nope, just returning 0 now instead of pulling out the decoder
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/decoder.hpp, lines 345-363
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line345>
> >
> > add asserts for failure?
well failure gets triggered in decode(), then on message begin the assert will check for failure, failure cannot be true in these functions
> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/decoder.hpp, line 372
> > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line372>
> >
> > new line
done
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10643
-----------------------------------------------------------
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10643
-----------------------------------------------------------
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment22758>
make this virtual
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment22799>
i'm guessing u made this public to make it visible for testing? if yes, please add a comment saying so.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22759>
s/std::string/string
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22761>
Kill this
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22768>
removing trailing "/" is a good utility function to have in strings
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22771>
when does this happen?
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22774>
push this down to where its used
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22780>
I dont' completely understand what's happening here. Can you add more comments/docs please?
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment22783>
can't u use numify for this?
src/master/slaves_manager.cpp
<https://reviews.apache.org/r/6617/#comment22784>
ditto
src/tests/configurator_tests.cpp
<https://reviews.apache.org/r/6617/#comment22793>
i like this abstraction!
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22787>
reorder?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22788>
new line
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22795>
for this test, why do u need something to the file?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22798>
are u supposed to test something here?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22800>
s/4/strlen("body")/
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22801>
not needed?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22803>
why these blocks?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22805>
you should probably factor this out into a helper function.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22804>
s/5/strlen("three")/
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22806>
it would be factor this out to a helper too.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22807>
not necessary
src/tests/utils.hpp
<https://reviews.apache.org/r/6617/#comment22809>
i see why you were creating parent directories here. we already have this function in stout/os, which acts like 'mkdir-p'. use that instead.
src/tests/utils.hpp
<https://reviews.apache.org/r/6617/#comment22808>
wait..dont we already have this abstraction in stout/os?
if so, can you use that instead?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment22812>
kill line
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment22815>
this seems like !isdir(), why do we need a different function?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6617/#comment22813>
kill line
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22816>
why static?
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22818>
need an assert(!decoder->failure) ?
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22820>
add asserts for failure?
third_party/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/6617/#comment22821>
new line
- Vinod Kone
On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2012, 1:37 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp c480bc6
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp 83d5daa
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/process/io.hpp 6a40b18
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/encoder.hpp ac4886e
> third_party/libprocess/src/process.cpp 78069bf
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Aug. 22, 2012, 1:37 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
this now has an http::get() which performs a _blocking_ get, I went down a rabbit-hole trying to write the async version so pairing on that part would be great! (can't quite re-use io::read as my TODO indicates).
this review is getting quite large, so we may want to sit down together and go through it, esp. if making get async
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am aaceee3
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/logging/logging.cpp 6909b0b
src/master/http.cpp c480bc6
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/utils.hpp 83d5daa
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/process/io.hpp 6a40b18
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp b1eceb3
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/encoder.hpp ac4886e
third_party/libprocess/src/process.cpp 78069bf
third_party/libprocess/src/statistics.cpp d05b327
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/
-----------------------------------------------------------
(Updated Aug. 17, 2012, 4:15 a.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Changes
-------
Big changes here! But lots of good cleanup around http request parsing.
I haven't cleaned up the File endpoints being exposed, will have to discuss the HttpRequest sending abstraction for tests with benh.
Description
-------
Implementing the file abstraction and http endpoints for file reading / browsing.
This addresses bug MESOS-255.
https://issues.apache.org/jira/browse/MESOS-255
Diffs (updated)
-----
src/Makefile.am b0cb6cc
src/files/files.hpp d0cab91
src/files/files.cpp d4080d4
src/logging/logging.cpp 6909b0b
src/master/http.cpp e783ac3
src/master/slaves_manager.cpp e25efd0
src/slave/http.cpp a1f7926
src/tests/configurator_tests.cpp c2f5aa0
src/tests/files_tests.cpp PRE-CREATION
src/tests/utils.hpp caf5926
src/webui/master/static/controllers.js 1606e64
third_party/libprocess/include/process/http.hpp 8424ca6
third_party/libprocess/include/stout/hashmap.hpp 51bdea0
third_party/libprocess/include/stout/json.hpp 25dbcf4
third_party/libprocess/include/stout/os.hpp b1eceb3
third_party/libprocess/include/stout/path.hpp 80d9bc6
third_party/libprocess/include/stout/stringify.hpp ad2f2fa
third_party/libprocess/src/decoder.hpp 105fe5d
third_party/libprocess/src/statistics.cpp d05b327
Diff: https://reviews.apache.org/r/6617/diff/
Testing
-------
Added files_tests.cpp
make check
Thanks,
Ben Mahler
Re: Review Request: Exposing files through http endpoints
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/path.hpp, line 20
> > <https://reviews.apache.org/r/6617/diff/1/?file=140343#file140343line20>
> >
> > &
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/files_tests.cpp, lines 254-255
> > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line254>
> >
> > Same comment as in files.cpp, unless you can't do that because C++ expects both expressions in the ternary if to have exactly the same types. In which case, I retract my comment above!
yeah that was the issue with ternary
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 130
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line130>
> >
> > s/val/value/
> >
> > Also, make both types be const&.
> >
> > Finally, this is too useful to sit locked away in this file! Please add it to process/http.hpp as an additional constructor for OK. Then you'd do:
> >
> > return OK(value, jsonp);
> >
> > Alternatively, you could add a top-level function that adds JSON to the body of any Response object (more general, but we can always do that later and make it specific for OK responses now).
Very clean, I like it! Added constructor to OK
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 190
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line190>
> >
> > Could just s/files/os::ls(path.get())/
done
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 194
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line194>
> >
> > Just an FYI: we try and construct C++ wrappers for C functions as much as possible. In your case you use multiple things returned from stat, so it's a bit harder. We've gotten away thus far with only needing one thing from stat at a time and thus have functions that wrap stat and just return that one thing.
If you don't object to double stat'ing for each non-dir in the listing, I could add an os::fileSize or something?
Otherwise, I'll leave as is unless you have alternatives?
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 195
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line195>
> >
> > Some sort of log warning would be nice.
Added, but why? The file could have simply been deleted since we did the listing in which case we really wouldn't care. But I guess just in case something else went wrong
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, lines 204-205
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line204>
> >
> > file.values["dir"] = S_ISDIR(s.st_mode) ? JSON::True() : JSON::False();
Sadly this does not work due to the typing.
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 225
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line225>
> >
> > I believe this breaks the webui (master/slave logs), so it would be great if this commit included that fix. Also, I'd like us to have a consistent query "key" for both 'browse' and 'read' (having to remember one keyword will be much better in the long run). I had chosen "name" because that matches with what you do when you "attach". However, I'd be okay (and perhaps is better) to do "path" instead, i.e.:
> >
> > ip:port/files/browse?path=...
> > ip:port/files/read?path=...
I went for clarity (file vs dir) but I like the consistency of path, fixed! (Path also makes more sense from the client side, like the ui etc)
Also thanks for catching the webui thing, please look the fixes over in controllers.js :)
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 440
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line440>
> >
> > It can not, because then we'd have to expose FilesProcess to/in files.hpp.
k
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/files_tests.cpp, line 245
> > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line245>
> >
> > You could throw this in a header and use it in files.cpp as well as here.
done, put it in files.hpp but it feels a bit out of place..
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 407
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line407>
> >
> > Try<string> result => Result<string> result ... how fitting.
hm? os::realpath returns a Try<string>
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/files_tests.cpp, line 45
> > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line45>
> >
> > s/val/value/
> >
> > Also, this should really be from an implementation of operator << (ostream, ...). Then you can just do: 'stringify(value)' below and it will convert the JSON object to a string. I like that better.
so looking at json.hpp the Value is not a struct, but rather a boost::variant typedef.. so I'm not so sure how to implement the << operator for that?
Seems like this was the original reason for the recursive Renderer in the first place..
So.. I tried to implement a stringify template override for JSON::Value but couldn't get the compiler to like it
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/configurator_tests.cpp, line 133
> > <https://reviews.apache.org/r/6617/diff/1/?file=140339#file140339line133>
> >
> > There is already a mkdir in stout. I'd prefer:
> >
> > mkdir("conf2") || FAIL() << "Failed to create directory 'conf2'";
> >
> > Otherwise mkDir has a nasty side-effect of failing without you realizing.
I noticed these, but sadly it doesn't fit on one line like that, hence why I made the test helpers (mkdir and writeFile) to fail the test on error under the hood.
I agree it's clunky to have the helpers here, but it's also really clunky to error check every mkdir and write in the tests (especially since it requires an if)
maybe if I do something like:
if (mkdir("1", 0755) || mkdir("2", 0755) || mkdir("3", 0755) {
FAIL() << "..."; // but this doesn't know which mkdir failed.
}
Thoughts? Right now I've renamed the test helpers to match the os::_ names but with cleaner signatures and they fail under the hood.
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/configurator_tests.cpp, line 134
> > <https://reviews.apache.org/r/6617/diff/1/?file=140339#file140339line134>
> >
> > There is also a "write" in stout/os.hpp.
> >
> > Try<bool> result = os::write("conf2/mesos.conf",
> > ....);
> >
> > result.isSome() || FAIL() << "" << result.error();
similarly to above, PTAL and let me know your thoughts
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, lines 157-158
> > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line157>
> >
> > Eventually it would be cool to have some abstractions for getting query key=value pairs out easily. Keep that in the back of your mind. ;)
done!! The request.query is now a map of the query <k,v> pairs. Now this review is deleting a lot of code.
Also, see the Option<V> get() method added to hashmap.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10321
-----------------------------------------------------------
On Aug. 17, 2012, 4:15 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2012, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am b0cb6cc
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/logging/logging.cpp 6909b0b
> src/master/http.cpp e783ac3
> src/master/slaves_manager.cpp e25efd0
> src/slave/http.cpp a1f7926
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp caf5926
> src/webui/master/static/controllers.js 1606e64
> third_party/libprocess/include/process/http.hpp 8424ca6
> third_party/libprocess/include/stout/hashmap.hpp 51bdea0
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/path.hpp 80d9bc6
> third_party/libprocess/include/stout/stringify.hpp ad2f2fa
> third_party/libprocess/src/decoder.hpp 105fe5d
> third_party/libprocess/src/statistics.cpp d05b327
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Exposing files through http endpoints
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6617/#review10321
-----------------------------------------------------------
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment21882>
Alphabetize please.
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment21883>
Space in between 'process' and 'stout' headers please.
src/files/files.hpp
<https://reviews.apache.org/r/6617/#comment22154>
I'd prefer not to expose these just for tests. Here are my suggestions:
(1) Put the FilesProcess class declaration in files/process.hpp and have files/files.cpp include files/files.hpp and files/process.hpp and put all the implementations there. Then you can directly test a FilesProcess via dispatches. The downside here is a little less opportunity for the compiler to optimize and slightly more verbose tests (need to do spawn, dispatch, wait, terminate).
(2) Add an abstraction for "submitting" an http::Request. Since a FilesProcess is currently always at a well known id (i.e., "files") you can easily create a Request for it (subsequent abstractions may require exposing their PID so that we can know where to send requests). This will be INCREDIBLY useful for testing lots of different current and future HTTP endpoints.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22155>
We've been including C headers first, then C++, then our own (i.e., those with "" not <>).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22158>
No longer need this! ;)
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22156>
I'm guessing this was for access to browse and read. If you decided to do (1) mentioned above just make browse and read be public. So kill this regardless.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22157>
s/val/value/
Also, make both types be const&.
Finally, this is too useful to sit locked away in this file! Please add it to process/http.hpp as an additional constructor for OK. Then you'd do:
return OK(value, jsonp);
Alternatively, you could add a top-level function that adds JSON to the body of any Response object (more general, but we can always do that later and make it specific for OK responses now).
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22159>
Eventually it would be cool to have some abstractions for getting query key=value pairs out easily. Keep that in the back of your mind. ;)
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22160>
This should be a BadRequest("Expecting 'dir=value' in query.\n").
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22162>
Only return NotFound if the file actually wasn't found. Have resolve return a Result instead of a Try.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22163>
Could just s/files/os::ls(path.get())/
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22165>
Just an FYI: we try and construct C++ wrappers for C functions as much as possible. In your case you use multiple things returned from stat, so it's a bit harder. We've gotten away thus far with only needing one thing from stat at a time and thus have functions that wrap stat and just return that one thing.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22164>
Some sort of log warning would be nice.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22166>
Kill.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22167>
file.values["dir"] = S_ISDIR(s.st_mode) ? JSON::True() : JSON::False();
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22168>
I believe this breaks the webui (master/slave logs), so it would be great if this commit included that fix. Also, I'd like us to have a consistent query "key" for both 'browse' and 'read' (having to remember one keyword will be much better in the long run). I had chosen "name" because that matches with what you do when you "attach". However, I'd be okay (and perhaps is better) to do "path" instead, i.e.:
ip:port/files/browse?path=...
ip:port/files/read?path=...
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22169>
BadRequest.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22161>
You should return a Result here. That way you can actually distinguish between NotFound and InternalServerError.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22171>
Try<string> result => Result<string> result ... how fitting.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22172>
Yes, my bad. Please delete.
src/files/files.cpp
<https://reviews.apache.org/r/6617/#comment22173>
It can not, because then we'd have to expose FilesProcess to/in files.hpp.
src/tests/configurator_tests.cpp
<https://reviews.apache.org/r/6617/#comment22174>
There is already a mkdir in stout. I'd prefer:
mkdir("conf2") || FAIL() << "Failed to create directory 'conf2'";
Otherwise mkDir has a nasty side-effect of failing without you realizing.
src/tests/configurator_tests.cpp
<https://reviews.apache.org/r/6617/#comment22175>
There is also a "write" in stout/os.hpp.
Try<bool> result = os::write("conf2/mesos.conf",
....);
result.isSome() || FAIL() << "" << result.error();
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22178>
s/val/value/
Also, this should really be from an implementation of operator << (ostream, ...). Then you can just do: 'stringify(value)' below and it will convert the JSON object to a string. I like that better.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22176>
Newline.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22177>
Two newlines between tests please (here and rest of file).
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22181>
You could throw this in a header and use it in files.cpp as well as here.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/6617/#comment22180>
Same comment as in files.cpp, unless you can't do that because C++ expects both expressions in the ternary if to have exactly the same types. In which case, I retract my comment above!
third_party/libprocess/include/stout/path.hpp
<https://reviews.apache.org/r/6617/#comment22182>
&
third_party/libprocess/include/stout/path.hpp
<https://reviews.apache.org/r/6617/#comment22183>
s/ret/result/
- Benjamin Hindman
On Aug. 15, 2012, 2:48 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6617/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2012, 2:48 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> Implementing the file abstraction and http endpoints for file reading / browsing.
>
>
> This addresses bug MESOS-255.
> https://issues.apache.org/jira/browse/MESOS-255
>
>
> Diffs
> -----
>
> src/Makefile.am b0cb6cc
> src/files/files.hpp d0cab91
> src/files/files.cpp d4080d4
> src/tests/configurator_tests.cpp c2f5aa0
> src/tests/files_tests.cpp PRE-CREATION
> src/tests/utils.hpp caf5926
> third_party/libprocess/include/stout/json.hpp 25dbcf4
> third_party/libprocess/include/stout/path.hpp 80d9bc6
>
> Diff: https://reviews.apache.org/r/6617/diff/
>
>
> Testing
> -------
>
> Added files_tests.cpp
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>