mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
Date Thu, 09 Jul 2015 19:19:10 GMT

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



3rdparty/libprocess/src/process.cpp (lines 2815 - 2819)
<https://reviews.apache.org/r/30032/#comment144432>

    Any reason we didn't convert os::stat::mtime to return a Time?
    
    The only other user of os::stat::mtime does the same messy conversion:
    
    ```
    Future<Nothing> Slave::garbageCollect(const string& path)
    {
      Try<long> mtime = os::stat::mtime(path);
      if (mtime.isError()) {
        LOG(ERROR) << "Failed to find the mtime of '" << path
                   << "': " << mtime.error();
        return Failure(mtime.error());
      }
    
      // It is unsafe for testing to use unix time directly, we must use
      // Time::create to convert into a Time object that reflects the
      // possibly advanced state of the libprocess Clock.
      Try<Time> time = Time::create(mtime.get());
      CHECK_SOME(time);
    ```



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

    Shouldn't this say if the file hasn't been modified since the requested time? Note that
a < comparison becomes possible when dealing with Time rather than time_t.



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

    Why did you need the {} here?



3rdparty/libprocess/src/process.cpp (lines 2839 - 2846)
<https://reviews.apache.org/r/30032/#comment144439>

    Any reason to prefer -1 special cases here to just using Try<Time> and handling
errors?



3rdparty/libprocess/src/process.cpp (lines 2852 - 2859)
<https://reviews.apache.org/r/30032/#comment144429>

    Why isn't this in the `if (modified)` branch below?



3rdparty/libprocess/src/process.cpp (lines 2857 - 2858)
<https://reviews.apache.org/r/30032/#comment144430>

    Mind lining this up according to the style guide and the code around you?



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

    How about s/cache/HttpCache/ ?
    
    We should probably start pulling out http server tests.



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

    std::string here but string above? Mind removing the std:: qualifiers?



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1726 - 1743)
<https://reviews.apache.org/r/30032/#comment144442>

    Yikes, could we add library support for parsing into Time to clean this up? Looks like
it could be a lot cleaner.



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1747 - 1751)
<https://reviews.apache.org/r/30032/#comment144447>

    Mind just using AWAIT_EXPECT_RESPONSE_STATUS_EQ to clean this up a bit? Ditto for the
other cases here.


- Ben Mahler


On June 17, 2015, 3:42 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> -----------------------------------------------------------
> 
> (Updated June 17, 2015, 3:42 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