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