-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30032/#review85183
-----------------------------------------------------------



3rdparty/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/30032/#comment136697>

    s/class/value
    ?



3rdparty/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/30032/#comment136700>

    If it is expected that time values can either be converted or not and it is 
normal if they cannot, then Option is fine. It seems to me that it should be 
abnormal if a time value cannot be formatted, right? So I would suggest to use 
Try instead of Option.
    
    Can we maybe use this function in time.hpp?
    
        // Outputs the time in RFC 3339 Format.
        inline std::ostream& operator << (std::ostream& stream, const Time& 
time)
    
    Ideally yes. If not, your function should be added to time.hpp.



3rdparty/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/30032/#comment136706>

    Shouldn't we cast to the exact type gmtime_r expects to avoid a potential 
warning? Yes, time_t may be declared by typedef to be a long in the end, but we 
should not rely on that.
    
    const time_t secs = ...



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/30032/#comment136704>

    1. Again, it seems to be an error if the conversion fails, not a normal 
case. => Try
    
    2. long => time_t



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/30032/#comment136711>

    Suggestion: You can use stat::mtime() from stout for now. And leave a TODO 
regarding Path.
    
    Later, finish https://reviews.apache.org/r/34392/. Then add a review that 
fixes every place Path could be used.



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/30032/#comment136707>

    Can you really be sure that no error occurs in Duration::get()? Maybe so, 
in practice in all likelyhood, but why not make sure and check? (For instance, 
Path::mtime() might be altered in the future without us anticipating this here 
and it might have a bug then.)



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/30032/#comment136708>

    The way this reads is an extra reason to move ::format() to a proper place. 
See above.



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/30032/#comment136712>

    This function can fail. Error handling, please.


- Bernd Mathiske


On May 26, 2015, 7:41 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 7:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
>     https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 
> bba62b393dc863e724cb602b1504eb6517ae9730 
>   3rdparty/libprocess/src/process.cpp 
> e3de3cd6b536aaaf59784360aed546512dd04dc9 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 67e582cc250a9767a389e2bd0cc68985477f3ffb 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to