> On July 9, 2015, 9:19 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 2815-2819 > > <https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2815> > > > > 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); > > ``` > > Till Toenshoff wrote: > One reason could be that os::stat::mtime lives in stout whereas Time > lives in libprocess, no?
Till is right, it is the only reason. `os::stat::mtime` is part of stout while `Time` is part of libprocess. We look into moving `Time` to stout, but there is a dependency to the clock which forbids the transition. > On July 9, 2015, 9:19 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, line 2833 > > <https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2833> > > > > Why did you need the {} here? It initializes a struct to be zero. It is equivalent to do `memset(&clientMTime, 0, sizeof(clientMTime))`, although my original patch called for using the more standard `= {0}` some previous reviewer suggested to change it to the version in the patch. > On July 9, 2015, 9:19 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 2839-2846 > > <https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2839> > > > > Any reason to prefer -1 special cases here to just using Try<Time> and > > handling errors? -1 is the value returned in error by `std::mktime`. As mentioned before, we couldn't use `Time`. Though it is possible to create a `Time` from a `time_t` initialized with `mktime`. I feel it just adds more points of failure. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review91149 ----------------------------------------------------------- On June 17, 2015, 5: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, 5: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 > >
