Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cookbook][Cache] Added config example for Varnish 4.0 #4280

Merged
merged 1 commit into from
Oct 28, 2014

Conversation

thierrymarianne
Copy link

Q A
Doc fix? [yes]
New docs? [no]
Applies to [all]
Fixed tickets []

After upgrading to Varnish 4.0, I've noticed the documentation was not really up-to-date anymore:

Would this addition be enough in order to consider the documentation up-to-date in accordance with Varnish 4.0 upgrading guidelines?

*/
sub vcl_backend_response {
/*
Check for ESI acknowledgement
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps you want to make this // instead or a full docblock, though tbh not sure the cs on varnish

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

@thierrymarianne thierrymarianne force-pushed the add_config_for_varnish_4.0 branch 2 times, most recently from e176347 to 507fd72 Compare September 29, 2014 22:53
@thierrymarianne
Copy link
Author

@cordoval, Thank you for the quick feedback, do you believe it could be satisfying enough?

@cordoval
Copy link
Contributor

imo is ok 👍 though i am not @wouterj or @xabbuh 😊

@thierrymarianne
Copy link
Author

@cordoval Thanks! I look forward to hearing from them ^_^

@wouterj
Copy link
Member

wouterj commented Sep 30, 2014

Related issue #4176

@dbu since you wanted to work on this chapter and varnish in general, can you have a look at this PR and share your thoughts?

@thierrymarianne
Copy link
Author

@dbu Having a tab for each version of Varnish would be much better!

We could even offer a docker image running the proper version of varnish with corresponding configuration files (strictly inspired from the documentation) as well :)


set beresp.do_esi = true;
}
// By default Varnish ignores Cache-Control: nocache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought they changed that in varnish 4, at least for cache-control. not sure about the pragma, can you check what / if any of the below are still needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should not be there, I should have gone from a blank page ... Sorry for that :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure about no-cache in pragma and about private, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check it out in the documentation. However, the best I could do would be to run the changes with the limited application I have and then revert to you with a mitigated opinion. I'm not sure it would cover all cases (and I don't think the documentation aims at covering it up or there would not be a Varnish documentation, right?).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu I've replaced no-cacheand pragma with beresp.http.X-No-Cache.

@dbu
Copy link
Contributor

dbu commented Oct 1, 2014

yeah the tabs for varnish config would be really cool (/me looking at @fabpot)

i intend to review the documentation and clean up what is on http://foshttpcache.readthedocs.org/ and what belongs into the symfony documentation itself. in the FOSHttpCache we have configuration fragments that we include in the doc which are used in functional tests. i think wouter was talking about trying to find a way to test configuration examples from the doc. this would be a particularly challenging bit to test. not sure how much its worth the effort or if we rather keep the examples very minimal. if you want to provide a demo docker setup with a playground for varnish features, that would be cool. for a workshop, i did a vagrant setup (plain php examples without symfony) i can share with you if you want.

@thierrymarianne thierrymarianne force-pushed the add_config_for_varnish_4.0 branch from 507fd72 to df2086e Compare October 1, 2014 07:09
@thierrymarianne
Copy link
Author

@dbu I would love to put together something like what you've suggested, a Docker image, which could be pulled and ran in order to check responses for specific request given some configuration 👍

}
if (beresp.http.Pragma ~ "no-cache" ||
beresp.http.Cache-Control ~ "no-cache" ||
beresp.http.Cache-Control ~ "private") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think some of these are not needed as they are the default behaviour. https://www.varnish-cache.org/docs/4.0/whats-new/upgrading.html#default-builtin-vcl-changes

and i would keep a comment similar to the one above that varnish 4 ignores the header by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu A comment similar to varnish 3.0 comment has been added.

@xabbuh
Copy link
Member

xabbuh commented Oct 2, 2014

@thierrymarianne @dbu will something like the following be sufficient?

.. configuration-block::

    .. code-block:: varnish2

        // ...

    .. code-block:: varnish3

        // ...

    .. code-block:: varnish4

        // ...

@thierrymarianne
Copy link
Author

@xabbuh To my mind, it would be more than perfect.

@dbu
Copy link
Contributor

dbu commented Oct 2, 2014

yep, exactly! (i would swap the order to show the current varnish by default and older versions on the not visible tabs, but whatever)

i wonder if this could be generic tabs rather than code tabs. then the content could specify the tab titles, in case we have other situations with alternatives. then we would still need to mark the tab content as code - but could also have varnish version specific comments (e.g. the doc links) inside the tab.

@xabbuh
Copy link
Member

xabbuh commented Oct 3, 2014

see fabpot/sphinx-php#20 and #4287

@thierrymarianne
Copy link
Author

@xabbuh Varnish has its own DSL : https://www.varnish-cache.org/trac/wiki/VCL

@xabbuh
Copy link
Member

xabbuh commented Oct 3, 2014

@thierrymarianne Yeah, but unfortunately it's not supported by Pygments (which is used for rendering code blocks). So, I was wondering if we could use Ruby to get some syntax highlighting since the syntax seems to be similar.

@thierrymarianne
Copy link
Author

@xabbuh Sure, if it provides a good enough syntax highlighting!

@xabbuh
Copy link
Member

xabbuh commented Oct 3, 2014

At least, it looks good locally. If you don't know any difference that may break it.

@thierrymarianne
Copy link
Author

👍

@dbu
Copy link
Contributor

dbu commented Oct 6, 2014

cool. are you going to update to the new tabs feature now? or do you want to leave that for later (= for me)?

we still have the open question which cache directives we need to explicitly track vs those that are respected by default in varnish 4.

@thierrymarianne
Copy link
Author

@dbu Sure (I intend to integrate the tabs feature) !

@xabbuh
Copy link
Member

xabbuh commented Oct 6, 2014

@thierrymarianne Note that this will only work once #4287 is merged.

@thierrymarianne
Copy link
Author

@xabbuh Well noted!

@thierrymarianne thierrymarianne force-pushed the add_config_for_varnish_4.0 branch from c58a6b6 to f3b7394 Compare October 19, 2014 09:29
@thierrymarianne
Copy link
Author

@xabbuh The blank line has been added. Thanks for the reminder!

@dbu
Copy link
Contributor

dbu commented Oct 20, 2014

i did not run them in varnish, but they seem to be correct. lets merge and we can still fix later on if people discover issues.

@weaverryan
Copy link
Member

Thanks so much everyone - this is a really great update in both content and readability. Cheers!

@weaverryan weaverryan merged commit f3b7394 into symfony:2.3 Oct 28, 2014
weaverryan added a commit that referenced this pull request Oct 28, 2014
…(thierrymarianne)

This PR was merged into the 2.3 branch.

Discussion
----------

[Cookbook][Cache] Added config example for Varnish 4.0

| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [all]
| Fixed tickets | []

After upgrading to `Varnish 4.0`, I've noticed the documentation was not really up-to-date anymore:

 * according to https://www.varnish-cache.org/docs/4.0/whats-new/upgrading.html#req-not-available-in-vcl-backend-response,

  `beresp` is available in `vcl_backend_response`

 * according to https://www.varnish-cache.org/docs/4.0/whats-new/upgrading.html#hit-for-pass-objects-are-created-using-beresp-uncacheable,

   `hit_for_pass` objects are created via `beresp.uncacheable`

Would this addition be enough in order to consider the documentation up-to-date in accordance with `Varnish 4.0` upgrading guidelines?

Commits
-------

f3b7394 Added config example for Varnish 4.0
@weaverryan
Copy link
Member

FYI - I just reverted this at sha: d07d5fe

I assumed that the sphinx-php extensions had been updated, but it looks like it hadn't, so the docs broke entirely. Let's get that fixed and then re-add this.

/cc @javiereguiluz @fabpot

@javiereguiluz
Copy link
Member

@weaverryan please tell me if this is correct: thanks to this PR sent by @xabbuh we could add the following to the Sphinx-PHP extensions to make this document work:

config_block = {
    'varnish4': 'Ruby',
    'varnish3': 'Ruby',
    'varnish2': 'Ruby'
}

@dbu
Copy link
Contributor

dbu commented Oct 29, 2014

@xabbuh
Copy link
Member

xabbuh commented Oct 29, 2014

@javiereguiluz That's right. But did you update the submodules on the build server?

@thierrymarianne
Copy link
Author

@weaverryan @xabbuh @dbu Sorry for the inconvenience. I'm afraid I've been to optimistic on this one. Next time, I'll make sure I could reproduce the end product on my end without guessing.

@xabbuh
Copy link
Member

xabbuh commented Oct 29, 2014

@thierrymarianne I don't think it's your fault. It seems to be that the build process doesn't update the Git submodules and therefore uses an older version of the Sphinx extensions.

@thierrymarianne
Copy link
Author

@xabbuh Let us say we all have our share of responsibility. Perhaps, we'll have a chance to discuss it in Madrid(?)

@wouterj
Copy link
Member

wouterj commented Oct 29, 2014

@thierrymarianne this is a fault on the build process of the docs. They don't use the configuration and sphinx extension included in this repo, but a custom one. It was not completely in sync, causing this to break the docs.

So, don't worry. This has nothing to do with you, if you render this locally everything will be correct.

@stof
Copy link
Member

stof commented Oct 29, 2014

@javiereguiluz couldn't the build process use the submodule of this repo for the extension, so that things working for the doc team and for Travis are also working when deploying ?

@weaverryan
Copy link
Member

+1 - we've had a problem for a while with the sphinx-php submodule not actually updating to the latest version. I kind of took a "stab in the dark" at merging this and hoping for the best - that's my fault. But let's get this fixed up if we can :)

@wouterj
Copy link
Member

wouterj commented Oct 29, 2014

Also a big +1 for me.

I know you are working on improve the documentation build process, including rendering the docs seperately instead of rendering all other docs (bundles, CMF) into the core docs. It would be nice if you could use the submodule and sphinx config included in the repo.

@fabpot
Copy link
Member

fabpot commented Oct 29, 2014

I've deployed the new version of sphinx-php on the server, so everything should work fine now, right?

@wouterj
Copy link
Member

wouterj commented Oct 29, 2014

yes. @weaverryan let's give it another shot, shall we?

weaverryan added a commit that referenced this pull request Oct 29, 2014
This re-adds PR #4280, which was temporarily reverted while we waited
for the proper PHP extensions.
@weaverryan
Copy link
Member

I've just re-pushed this PR at sha: 23afdb3

Thanks @fabpot - I'll stop by the ZendCon Sensio booth later for a high-five (and a BlackFire.io demo if you'll give me one).

@xabbuh
Copy link
Member

xabbuh commented Oct 29, 2014

We will probably need another update for the best practices (see #4394).

weaverryan added a commit that referenced this pull request Oct 30, 2014
This reverts commit 23afdb3.

We thought we had the issue fixed, but apparently not quite yet - see #4280
@weaverryan
Copy link
Member

Hey guys!

Sorry for the issues - I just reverted this again at sha: b3d96b8. We're still seeing the same build error during the full build (which happens about right now each day).

I'll wait to see if we can get another attempted fix up there!

screen shot 2014-10-29 at 8 00 31 pm

@dbu
Copy link
Contributor

dbu commented Nov 3, 2014

would be nice if somebody who has access to the servers and the setup could track the thing down, rather than ryan having to break the live doc to test things out ;-)

@weaverryan
Copy link
Member

I've created an issue - #4415 so that we don't lose this (since this PR is closed).

@dbu if it's helpful that I have access, then cool. If we get on top of the issue (e.g. if the deploy uses the git-submodule version that we commit), then that's cool for me too :).

Thanks!

weaverryan added a commit that referenced this pull request Nov 3, 2014
This reverts a revert, so really it re-adds #4280. See #4415 also.

This reverts commit b3d96b8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants