Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1757 - 1761)


I think the error was the lack of an await here.


- Alexander Rojas


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2815-2819
> > 
> >
> > 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 Slave::garbageCollect(const string& path)
> > {
> >   Try 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::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
> > 
> >
> > 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
> > 
> >
> > Any reason to prefer -1 special cases here to just using Try 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
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> >
> 
> Till Toenshoff wrote:
> Thanks for looking into this Ben - we will shortly propose a fix for the 
> current test break on Ubuntu and also most of these nits.

We decided to revert that commit instead for now until we can thorroughly fix 
this RR.


> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2815-2819
> > 
> >
> > 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 Slave::garbageCollect(const string& path)
> > {
> >   Try 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::create(mtime.get());
> >   CHECK_SOME(time);
> > ```

One reason could be that os::stat::mtime lives in stout whereas Time lives in 
libprocess, no?


- Till


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

> On July 9, 2015, 7:19 p.m., Ben Mahler wrote:
> >

Thanks for looking into this Ben - we will shortly propose a fix for the 
current test break on Ubuntu and also most of these nits.


- Till


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Ben Mahler]

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


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 Slave::garbageCollect(const string& path)
{
  Try 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::create(mtime.get());
  CHECK_SOME(time);
```



3rdparty/libprocess/src/process.cpp (line 2827)


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)


Why did you need the {} here?



3rdparty/libprocess/src/process.cpp (lines 2839 - 2846)


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



3rdparty/libprocess/src/process.cpp (lines 2852 - 2859)


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



3rdparty/libprocess/src/process.cpp (lines 2857 - 2858)


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



3rdparty/libprocess/src/tests/process_tests.cpp (line 1680)


How about s/cache/HttpCache/ ?

We should probably start pulling out http server tests.



3rdparty/libprocess/src/tests/process_tests.cpp (line 1711)


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



3rdparty/libprocess/src/tests/process_tests.cpp (lines 1726 - 1743)


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)


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/src/tests/process_tests.cpp (line 1680)


A short description on the target of this test would be terriffic -- will 
fix this while committing.


- Till Toenshoff


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Bernd Mathiske]

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

Ship it!


If/when RR 34703 ships first, please fix this TODO:

// TODO(arojas): Use Path.mtime() once it is available.

- Bernd Mathiske


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34703, 30032]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On June 17, 2015, 4 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2842
> > 
> >
> > Here we can output mtime.error().

This one is a bad place since here `strftime` failed, which is unrelated to 
`mtime`. I did find a better place though.


> On June 17, 2015, 4 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2861
> > 
> >
> > No need to check mtime.isError(). It is actually a bit confusing all 
> > the explanations above.

I know the comment above is confusing, however it appears in all the 
`dispatch(proxy, &HttpProxy::enqueue, _, _)` in the file.


- Alexander


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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


Changes
---

Addresses Bernd's review


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Bernd Mathiske]

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


Well done! An very nice test code.


3rdparty/libprocess/src/process.cpp (line 2815)


Since this review depends on RR 34307 that introduces Path:mtime() you 
should be using it already!



3rdparty/libprocess/src/process.cpp (line 2817)


That the time could not be parsed is not the only conceivable cause of 
error. For whatever reason, the asset could be gone, for example. Better to 
propagate the error from systemMtime.error().



3rdparty/libprocess/src/process.cpp (line 2818)


If we used a conditional operator (?) expression instead, mtime could be 
const.



3rdparty/libprocess/src/process.cpp (line 2822)


s/hasn't/has not
Or delete the sentence if you handle the next issue as suggested.



3rdparty/libprocess/src/process.cpp (line 2823)


Suggestion to rephrase this:

"These conditions must hold to establish that the file has not been 
modified since its most recent access:
1. The If-Modified-Since' header must be present in the request.
2. The local modification time must be determined here. 
3. That modification time must differ from the time provided by the above 
header.
An error is logged, but not propagated if any of this fails. Then the 
request simply proceeds as if the file had been modified."



3rdparty/libprocess/src/process.cpp (line 2826)


I'd move this line up before the comment.



3rdparty/libprocess/src/process.cpp (line 2838)


We could log if client == -1 happens.



3rdparty/libprocess/src/process.cpp (line 2842)


Here we can output mtime.error().



3rdparty/libprocess/src/process.cpp (line 2846)


Suggestion: "Provide the Last-Modified header to the client to facilitate 
its setting up a cache."



3rdparty/libprocess/src/process.cpp (line 2861)


No need to check mtime.isError(). It is actually a bit confusing all the 
explanations above.



3rdparty/libprocess/src/process.cpp (line 2862)


Delete this redundant comment.



3rdparty/libprocess/src/process.cpp (line 2862)


Delete this redundant comment.



3rdparty/libprocess/src/tests/process_tests.cpp (line 1734)


seconds



3rdparty/libprocess/src/tests/process_tests.cpp (line 1749)


s/Request/The request's modification
s/mismatch/does not match the



3rdparty/libprocess/src/tests/process_tests.cpp (line 1758)


See above.


- Bernd Mathiske


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34703, 30032]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated June 17, 2015, 4:08 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Forcing rebuild, since it failed thanks to flaky test.


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated June 16, 2015, 9:53 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Added missing break line.


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34703, 30032]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 3:27 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 15, 2015, 3:27 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 
> f919b997287435381dbe34cb5bfdf73641ebeb23 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated June 15, 2015, 5:27 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

User `Time::create` to create time objects. Removes unnecessary code.


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 (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
e47cc7afbc8110759edf25a2dc05d09eda25c417 
  3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 
  3rdparty/libprocess/src/tests/process_tests.cpp 
660af45e7fd45bdf5d43ad9aa54477fd94f87058 

Diff: https://reviews.apache.org/r/30032/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34703, 30032]

All tests passed.

- Mesos ReviewBot


On June 8, 2015, 3:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 8, 2015, 3:14 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 
> 51a00f5cf6a84413c8ab73a4e62e1814e9af2339 
>   3rdparty/libprocess/src/process.cpp 
> aadd7bb0ae12b93336900c76d8d5aaa4421ea198 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 0832c6266cea0fb6bdbd523921c1367959419af8 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated June 8, 2015, 5:14 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

rebased.


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 (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
51a00f5cf6a84413c8ab73a4e62e1814e9af2339 
  3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198 
  3rdparty/libprocess/src/tests/process_tests.cpp 
0832c6266cea0fb6bdbd523921c1367959419af8 

Diff: https://reviews.apache.org/r/30032/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34392, 34703, 30032]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On May 26, 2015, 6:15 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 334
> > 
> >
> > s/class/value
> > ?

Removed format from here.


> On May 26, 2015, 6:15 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 335
> > 
> >
> > 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.

Check review [34703](https://reviews.apache.org/r/34703/) which introduces 
exactly that.


> On May 26, 2015, 6:15 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 337
> > 
> >
> > 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 = ...

Check review [34703](https://reviews.apache.org/r/34703/) which addresses this 
issue.


- Alexander


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated May 27, 2015, 4:50 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Makes the changes requiered by Bernd's review.
Adds a dependency to review 34703.


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On May 20, 2015, 7:10 a.m., Nikita Vetoshkin wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 352
> > 
> >
> > strftime formatting is locale dependent. This example 
> > ```
> > #include 
> > #include 
> > #include 
> > 
> > 
> > int main() {
> > char outstr[200];
> > time_t t;
> > struct tm *tmp;
> > 
> > const char HTTP_DATE[] = "%a, %d %b %Y %T %Z";
> > 
> > setlocale(LC_ALL, "ru_RU.UTF-8");
> > 
> > t = time(NULL);
> > tmp = localtime(&t);
> > 
> > strftime(outstr, sizeof(outstr), HTTP_DATE, tmp);
> > 
> > printf("Result string is \"%s\"\n", outstr);
> >  
> > }
> > ```
> > on my laptop produces
> > ```
> > Result string is "??, 20 ??? 2015 10:04:43 YEKT"
> > ```
> > Don't know if it is an issue, maybe process should call 
> > `setlocale(LC_ALL, "C");` or add a TODO?
> 
> Nikita Vetoshkin wrote:
> I think this approuch should work regardless locale being used
> 
> ```
> #include 
> #include 
> #include 
> 
> 
> int main() {
>   char outstr[200];
> time_t t;
> struct tm *date;
> 
> const char *WEEK_DAYS[] = {"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", 
> "Sun"};
> const char *MONTHS[] = {
>   "Jan", "Feb",
>   "Mar", "Apr", "May",
>   "Jun", "Jul", "Aug",
>   "Sep", "Oct", "Nov",
>   "Dec"};
> const char FORMAT[] = "%s, %02d %s %d %02d:%02d:%02d GMT";
> 
> setlocale(LC_ALL, "ru_RU.UTF-8");
> 
> t = time(NULL);
> date = gmtime(&t);
> 
> snprintf(outstr, sizeof(outstr), FORMAT,
>   WEEK_DAYS[date->tm_wday],
>   date->tm_mday,
>   MONTHS[date->tm_mon],
>   date->tm_year + 1900,
>   date->tm_hour,
>   date->tm_min,
>   date->tm_sec);
> 
>   printf("Result string is \"%s\"\n", outstr);
> }
> ```
> 
> Alexander Rojas wrote:
> Yeah, the problem is, there are more places in the code base using 
> strftime, so I started a discussion in the dev list and see what happens 
> there.

Added your solution as part of the Time object serializers. You can check it 
[here](https://reviews.apache.org/r/34703/)


- Alexander


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34392, 30032]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Bernd Mathiske]

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



3rdparty/libprocess/include/process/http.hpp


s/class/value
?



3rdparty/libprocess/include/process/http.hpp


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


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


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


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


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


The way this reads is an extra reason to move ::format() to a proper place. 
See above.



3rdparty/libprocess/src/process.cpp


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On May 22, 2015, 5:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2708
> > 
> >
> > Why Result and not Try? Why not propagate the error from mtime?
> > 
> > Why snake_case and not camelCase?

1. The functions used within `time_utc` don't set errno nor set an error 
string. `None` is enough to continue.
1. The snake_case is used because it is consistent with all the functions in 
the `internal` namespace within this file.


> On May 22, 2015, 5:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2716
> > 
> >
> > Why not report the error here?

See above.


> On May 22, 2015, 5:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2724
> > 
> >
> > Can we call this in NotModified above? See also the comments there, 
> > which also apply here, since this seems to be the same code.

Moved the function to a static method within `NotModified`.


- Alexander


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


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated May 26, 2015, 4:41 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Bernd's reviews.


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Bernd Mathiske]

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


First pass.


3rdparty/libprocess/include/process/http.hpp


Alternatively you could just use a naked constant and then sizeof(buffer) 
below. Declaring the constant does not add any information.



3rdparty/libprocess/src/process.cpp


s/tc/tm

There seems to be no call to stat here, so how can this comment be correct?



3rdparty/libprocess/src/process.cpp


Why Result and not Try? Why not propagate the error from mtime?

Why snake_case and not camelCase?



3rdparty/libprocess/src/process.cpp


Why not report the error here?



3rdparty/libprocess/src/process.cpp


Can we call this in NotModified above? See also the comments there, which 
also apply here, since this seems to be the same code.



3rdparty/libprocess/src/tests/process_tests.cpp


Insert blank above comment.



3rdparty/libprocess/src/tests/process_tests.cpp


Insert blank above comment and below last line of which it applies to.


irst pass

- Bernd Mathiske


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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On May 20, 2015, 7:10 a.m., Nikita Vetoshkin wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 352
> > 
> >
> > strftime formatting is locale dependent. This example 
> > ```
> > #include 
> > #include 
> > #include 
> > 
> > 
> > int main() {
> > char outstr[200];
> > time_t t;
> > struct tm *tmp;
> > 
> > const char HTTP_DATE[] = "%a, %d %b %Y %T %Z";
> > 
> > setlocale(LC_ALL, "ru_RU.UTF-8");
> > 
> > t = time(NULL);
> > tmp = localtime(&t);
> > 
> > strftime(outstr, sizeof(outstr), HTTP_DATE, tmp);
> > 
> > printf("Result string is \"%s\"\n", outstr);
> >  
> > }
> > ```
> > on my laptop produces
> > ```
> > Result string is "??, 20 ??? 2015 10:04:43 YEKT"
> > ```
> > Don't know if it is an issue, maybe process should call 
> > `setlocale(LC_ALL, "C");` or add a TODO?
> 
> Nikita Vetoshkin wrote:
> I think this approuch should work regardless locale being used
> 
> ```
> #include 
> #include 
> #include 
> 
> 
> int main() {
>   char outstr[200];
> time_t t;
> struct tm *date;
> 
> const char *WEEK_DAYS[] = {"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", 
> "Sun"};
> const char *MONTHS[] = {
>   "Jan", "Feb",
>   "Mar", "Apr", "May",
>   "Jun", "Jul", "Aug",
>   "Sep", "Oct", "Nov",
>   "Dec"};
> const char FORMAT[] = "%s, %02d %s %d %02d:%02d:%02d GMT";
> 
> setlocale(LC_ALL, "ru_RU.UTF-8");
> 
> t = time(NULL);
> date = gmtime(&t);
> 
> snprintf(outstr, sizeof(outstr), FORMAT,
>   WEEK_DAYS[date->tm_wday],
>   date->tm_mday,
>   MONTHS[date->tm_mon],
>   date->tm_year + 1900,
>   date->tm_hour,
>   date->tm_min,
>   date->tm_sec);
> 
>   printf("Result string is \"%s\"\n", outstr);
> }
> ```

Yeah, the problem is, there are more places in the code base using strftime, so 
I started a discussion in the dev list and see what happens there.


- Alexander


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


On May 19, 2015, 6:20 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated May 19, 2015, 6:20 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
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Nikita Vetoshkin]

> On May 20, 2015, 5:10 a.m., Nikita Vetoshkin wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 352
> > 
> >
> > strftime formatting is locale dependent. This example 
> > ```
> > #include 
> > #include 
> > #include 
> > 
> > 
> > int main() {
> > char outstr[200];
> > time_t t;
> > struct tm *tmp;
> > 
> > const char HTTP_DATE[] = "%a, %d %b %Y %T %Z";
> > 
> > setlocale(LC_ALL, "ru_RU.UTF-8");
> > 
> > t = time(NULL);
> > tmp = localtime(&t);
> > 
> > strftime(outstr, sizeof(outstr), HTTP_DATE, tmp);
> > 
> > printf("Result string is \"%s\"\n", outstr);
> >  
> > }
> > ```
> > on my laptop produces
> > ```
> > Result string is "??, 20 ??? 2015 10:04:43 YEKT"
> > ```
> > Don't know if it is an issue, maybe process should call 
> > `setlocale(LC_ALL, "C");` or add a TODO?

I think this approuch should work regardless locale being used

```
#include 
#include 
#include 


int main() {
char outstr[200];
time_t t;
struct tm *date;

const char *WEEK_DAYS[] = {"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"};
const char *MONTHS[] = {
"Jan", "Feb",
"Mar", "Apr", "May",
"Jun", "Jul", "Aug",
"Sep", "Oct", "Nov",
"Dec"};
const char FORMAT[] = "%s, %02d %s %d %02d:%02d:%02d GMT";

setlocale(LC_ALL, "ru_RU.UTF-8");

t = time(NULL);
date = gmtime(&t);

snprintf(outstr, sizeof(outstr), FORMAT,
WEEK_DAYS[date->tm_wday],
date->tm_mday,
MONTHS[date->tm_mon],
date->tm_year + 1900,
date->tm_hour,
date->tm_min,
date->tm_sec);

printf("Result string is \"%s\"\n", outstr);
}
```


- Nikita


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


On May 19, 2015, 4:20 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated May 19, 2015, 4:20 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
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Nikita Vetoshkin]

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



3rdparty/libprocess/include/process/http.hpp


strftime formatting is locale dependent. This example 
```
#include 
#include 
#include 

int main() {
char outstr[200];
time_t t;
struct tm *tmp;

const char HTTP_DATE[] = "%a, %d %b %Y %T %Z";

setlocale(LC_ALL, "ru_RU.UTF-8");

t = time(NULL);
tmp = localtime(&t);

strftime(outstr, sizeof(outstr), HTTP_DATE, tmp);

printf("Result string is \"%s\"\n", outstr);
 
}
```
on my laptop produces
```
Result string is "??, 20 ??? 2015 10:04:43 YEKT"
```
Don't know if it is an issue, maybe process should call `setlocale(LC_ALL, 
"C");` or add a TODO?


- Nikita Vetoshkin


On May 19, 2015, 4:20 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated May 19, 2015, 4:20 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
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34392, 30032]

All tests passed.

- Mesos ReviewBot


On May 19, 2015, 4:20 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated May 19, 2015, 4:20 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
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated May 19, 2015, 6:20 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Solves issues from reviewers.


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 (updated)
-

  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

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On March 26, 2015, 5:59 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2854
> > 
> >
> > Result time = Path(response.path).mtime();
> 
> Alexander Rojas wrote:
> This cannot be implemented in terms on `Time`, since `Path` is part of 
> stout and `Time` belongs to libprocess.
> 
> Till Toenshoff wrote:
> Seems `Time` actually should be a stout class.
> 
> Alexander Rojas wrote:
> From benh:
> > Why is Time in libprocess? Because we have the ability to stop time via 
> the Clock, which means we don't want anyone to _get_ time from anyplace else. 
> If we put Time in stout then it's possible that someone could construct a 
> Time that wasn't controlled by the libprocess Clock which wouldn't let us 
> stop or advance time (per process) effectively. Make sense?

Solve it returning a long from `Path::mtime` which works as a wrapper for 
`stout::os::path::mtime`


- Alexander


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


On April 20, 2015, 1:58 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated April 20, 2015, 1:58 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 
> 07825b2b7195fe7fe752e8fda65b7f0a8b8b1f38 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On March 26, 2015, 5:59 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2854
> > 
> >
> > Result time = Path(response.path).mtime();
> 
> Alexander Rojas wrote:
> This cannot be implemented in terms on `Time`, since `Path` is part of 
> stout and `Time` belongs to libprocess.
> 
> Till Toenshoff wrote:
> Seems `Time` actually should be a stout class.

>From benh:
> Why is Time in libprocess? Because we have the ability to stop time via the 
> Clock, which means we don't want anyone to _get_ time from anyplace else. If 
> we put Time in stout then it's possible that someone could construct a Time 
> that wasn't controlled by the libprocess Clock which wouldn't let us stop or 
> advance time (per process) effectively. Make sense?


- Alexander


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


On April 20, 2015, 1:58 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated April 20, 2015, 1:58 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 
> 07825b2b7195fe7fe752e8fda65b7f0a8b8b1f38 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Till Toenshoff]

> On March 26, 2015, 4:59 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2854
> > 
> >
> > Result time = Path(response.path).mtime();
> 
> Alexander Rojas wrote:
> This cannot be implemented in terms on `Time`, since `Path` is part of 
> stout and `Time` belongs to libprocess.

Seems `Time` actually should be a stout class.


- Till


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


On April 20, 2015, 11:58 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated April 20, 2015, 11:58 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 
> 07825b2b7195fe7fe752e8fda65b7f0a8b8b1f38 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

> On March 26, 2015, 5:59 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2854
> > 
> >
> > Result time = Path(response.path).mtime();

This cannot be implemented in terms on `Time`, since `Path` is part of stout 
and `Time` belongs to libprocess.


- Alexander


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


On April 20, 2015, 1:58 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated April 20, 2015, 1:58 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 
> 07825b2b7195fe7fe752e8fda65b7f0a8b8b1f38 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>