This is an automatically generated e-mail. To reply, visit:

Well done! An very nice test code.

3rdparty/libprocess/src/process.cpp (line 2815)

    Since this review depends on RR 34307 that introduces Path:mtime() you 
should be using it already!

3rdparty/libprocess/src/process.cpp (line 2817)

    That the time could not be parsed is not the only conceivable cause of 
error. For whatever reason, the asset could be gone, for example. Better to 
propagate the error from systemMtime.error().

3rdparty/libprocess/src/process.cpp (line 2818)

    If we used a conditional operator (?) expression instead, mtime could be 

3rdparty/libprocess/src/process.cpp (line 2822)

    s/hasn't/has not
    Or delete the sentence if you handle the next issue as suggested.

3rdparty/libprocess/src/process.cpp (line 2823)

    Suggestion to rephrase this:
    "These conditions must hold to establish that the file has not been 
modified since its most recent access:
    1. The If-Modified-Since' header must be present in the request.
    2. The local modification time must be determined here. 
    3. That modification time must differ from the time provided by the above 
    An error is logged, but not propagated if any of this fails. Then the 
request simply proceeds as if the file had been modified."

3rdparty/libprocess/src/process.cpp (line 2826)

    I'd move this line up before the comment.

3rdparty/libprocess/src/process.cpp (line 2838)

    We could log if client == -1 happens.

3rdparty/libprocess/src/process.cpp (line 2842)

    Here we can output mtime.error().

3rdparty/libprocess/src/process.cpp (line 2846)

    Suggestion: "Provide the Last-Modified header to the client to facilitate 
its setting up a cache."

3rdparty/libprocess/src/process.cpp (line 2861)

    No need to check mtime.isError(). It is actually a bit confusing all the 
explanations above.

3rdparty/libprocess/src/process.cpp (line 2862)

    Delete this redundant comment.

3rdparty/libprocess/src/process.cpp (line 2862)

    Delete this redundant comment.

3rdparty/libprocess/src/tests/process_tests.cpp (line 1734)


3rdparty/libprocess/src/tests/process_tests.cpp (line 1749)

    s/Request/The request's modification
    s/mismatch/does not match the

3rdparty/libprocess/src/tests/process_tests.cpp (line 1758)

    See above.

- Bernd Mathiske

On June 16, 2015, 7:08 p.m., Alexander Rojas wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> -----------------------------------------------------------
> (Updated June 16, 2015, 7:08 p.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 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> a67a3afdb30d23eb1b265b04ae662f64e874b6c6 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> Diff: https://reviews.apache.org/r/30032/diff/
> Testing
> -------
> make check
> Thanks,
> Alexander Rojas

Reply via email to