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 Tue, 26 May 2015 16:15:34 GMT

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


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