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