mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
Date Wed, 17 Jun 2015 14:00:47 GMT

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


Well done! An very nice test code.


3rdparty/libprocess/src/process.cpp (line 2815)
<https://reviews.apache.org/r/30032/#comment140607>

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



3rdparty/libprocess/src/process.cpp (line 2817)
<https://reviews.apache.org/r/30032/#comment140605>

    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)
<https://reviews.apache.org/r/30032/#comment140606>

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



3rdparty/libprocess/src/process.cpp (line 2822)
<https://reviews.apache.org/r/30032/#comment140611>

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



3rdparty/libprocess/src/process.cpp (line 2823)
<https://reviews.apache.org/r/30032/#comment140613>

    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 header.
    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)
<https://reviews.apache.org/r/30032/#comment140610>

    I'd move this line up before the comment.



3rdparty/libprocess/src/process.cpp (line 2838)
<https://reviews.apache.org/r/30032/#comment140614>

    We could log if client == -1 happens.



3rdparty/libprocess/src/process.cpp (line 2842)
<https://reviews.apache.org/r/30032/#comment140609>

    Here we can output mtime.error().



3rdparty/libprocess/src/process.cpp (line 2846)
<https://reviews.apache.org/r/30032/#comment140615>

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



3rdparty/libprocess/src/process.cpp (line 2861)
<https://reviews.apache.org/r/30032/#comment140624>

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



3rdparty/libprocess/src/process.cpp (line 2862)
<https://reviews.apache.org/r/30032/#comment140625>

    Delete this redundant comment.



3rdparty/libprocess/src/process.cpp (line 2862)
<https://reviews.apache.org/r/30032/#comment140626>

    Delete this redundant comment.



3rdparty/libprocess/src/tests/process_tests.cpp (line 1734)
<https://reviews.apache.org/r/30032/#comment140628>

    seconds



3rdparty/libprocess/src/tests/process_tests.cpp (line 1749)
<https://reviews.apache.org/r/30032/#comment140631>

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



3rdparty/libprocess/src/tests/process_tests.cpp (line 1758)
<https://reviews.apache.org/r/30032/#comment140632>

    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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message