New vcl_backend_refresh method #3994
Conversation
walid-git
force-pushed
the
vcl_backend_refresh
branch
from
319b37d
to
432de57
Compare
|
You need to update |
|
This is also missing the |
bin/varnishd/builtin.vcl
Outdated
| sub vcl_backend_refresh { | ||
| return (merge); | ||
| } |
There was a problem hiding this comment.
I'm having second thoughts here, and I'm wondering whether we should generalize this subroutine to whenever there is a stale object involved.
sub vcl_backend_refresh {
if (obj_stale.status == 200 && beresp.status == 304) {
return (merge);
}
if (beresp.status == 304) {
return (error); # until we refine error statuses
}
return (beresp);
}Of course, with a pinch of splitting.
There was a problem hiding this comment.
@dridi can you please motivate this suggestion? What else would you do with the stale object besides delivering it as-is (for grace mode) or use it for a refresh?
walid-git
force-pushed
the
vcl_backend_refresh
branch
3 times, most recently
from
5722162
to
68dda70
Compare
This commit introduces a new sub vcl_backend_refresh method intended to allow allow a more flexible approach on object revalidation.
walid-git
force-pushed
the
vcl_backend_refresh
branch
from
68dda70
to
a14d4fd
Compare
| case VCL_RET_ERROR: | ||
| /* FALLTHROUGH */ | ||
| case VCL_RET_ABANDON: | ||
| /* FALLTHROUGH */ | ||
| case VCL_RET_FAIL: | ||
| /* FALLTHROUGH */ | ||
| case VCL_RET_RETRY: | ||
| skip_vbr = 1; | ||
| break; |
There was a problem hiding this comment.
You can simply do this:
case VCL_RET_ERROR:
case VCL_RET_ABANDON:
case VCL_RET_FAIL:
case VCL_RET_RETRY:
skip_vbr = 1;
break;It shouldn't trigger a warning.
There was a problem hiding this comment.
I remember doing that on another PR and getting CCI failing compilation because of -Wimplicit-fallthrough
There was a problem hiding this comment.
I think that only happens if you got statements in-between without a break.
There was a problem hiding this comment.
Oh right, I can test it then
| case HDR_OBJ_STALE: | ||
| /* FALLTHROUGH */ | ||
| case HDR_OBJ: | ||
| hp = NULL; | ||
| break; |
There was a problem hiding this comment.
case HDR_OBJ:
case HDR_OBJ_STALE:
hp = NULL;
break;
bin/varnishd/builtin.vcl
Outdated
| if (!obj_stale.http.Last-Modified && | ||
| !obj_stale.http.ETag) { |
There was a problem hiding this comment.
You are using obj_stale before it exists.
There was a problem hiding this comment.
I'm not sure to understand, we only get into vcl_backend_refresh when we already have a stale_oc
There was a problem hiding this comment.
The variable is introduced in VCL in the last commit of the current series.
obj_stale variable gives access to the stale object we had in cache when doing a 304 revalidation. It is only readable from vcl_backend_refresh subroutine.
walid-git
force-pushed
the
vcl_backend_refresh
branch
from
a14d4fd
to
45728ff
Compare
|
bugwash: #4026 should be addressed first |
Named after vcl_backend_refresh from varnishcache#3994.
Named after vcl_backend_refresh from varnishcache#3994.
This PR introduces the new vcl_backend_refresh method discussed during VDD23Q3.
See: #3102 (comment) for more context.