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

Vary header cache #298

Closed
wants to merge 15 commits into from
Closed

Vary header cache #298

wants to merge 15 commits into from

Conversation

FSchumacher
Copy link
Contributor

First try on an implementation to support vary header as asked for in
https://bz.apache.org/bugzilla/show_bug.cgi?id=61176

Do not merge.

I haven't written tests yet, as I am not sure, if the code is going in the right direction.

The idea at the moment is to store two entries in the cache, when a vary header is found in the response headers. One entry for the original url with the name of the vary header set and one entry for the url+var-header-name+vary-header-value (of the request).

The code makes it clearer, that the two http samplers with slightly different implementation details make it harder to implement this. For example both samplers have different notions of how the headers are implemented.

…mode.

We are not not doing any real work here and it simplifies our code.
This should make the code a bit more readable.
Extract the various methods for calculation of the expiration date into
smaller methods in an attempt to make code more readable.
Copy link

@undera undera left a comment

Choose a reason for hiding this comment

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

First of all - thanks for this initiative. Also I appreciate the refactorings made, project needs them a lot.

One thing that I see is not sufficient is lack of multi-header handling for Vary. RFC allows that and I saw real servers working like that (https://tools.ietf.org/html/rfc7231#section-7.1.4).
IMO implementation has to use all of headers listed in Vary as part of cache entry key.

And yes, we need unit tests to see how it all will behave as integral.

@@ -152,22 +197,18 @@ private boolean hasVaryHeader(URLConnection conn) {
* result to decide if result is cacheable
*/
public void saveDetails(HttpResponse method, HTTPSampleResult res) {
Copy link

Choose a reason for hiding this comment

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

This method contents looks like duplicate of another saveDetails. I'd suggest refactoring common code into separate method to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is old code and a result of the different implementations of the http samplers. What refactoring do you have in mind?

@@ -626,6 +627,19 @@ protected HTTPSampleResult sample(URL url, String method, boolean areFollowingRe
}
}

private Header[] getHeaders(HeaderManager headerManager) {
Copy link

Choose a reason for hiding this comment

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

I'd make this protected for extensibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of extracting it into a utility class.

Copy link
Collaborator

@vlsi vlsi left a comment

Choose a reason for hiding this comment

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

I'm not sure on the functional side of the feature, however there is a couple of performance-wise issues.

return null;
}
for (String headerLine: reqHeaders.split("\n")) {
if (headerLine.startsWith(headerName + ": ")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move string concatenation out of a loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking of splitting the headerLine, so that I can easier compare the headerName ignoring the case.

And as undera pointed out, a vary header can have multiple values (separated by comma), so this logic will have to be changed anyway.

if (varyHeader != null) {
log.debug("Set entry into cache for url {} and vary {} ({})", url,
varyHeader,
varyUrl(url, varyHeader.getLeft(), varyHeader.getRight()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add isDebugEnabled check to prevent varyUrl computation in debug disabled case.

@pmouawad
Copy link
Contributor

Thanks Felix for PR.
I reviewed it, it looks good to me.
Maybe since we're making API modifications we should start thinking about:

Thanks

@undera
Copy link

undera commented Jun 29, 2017

Great progress, Felix!
Now we need to test it against real websites with Vary header, right?

@FSchumacher
Copy link
Contributor Author

@undera there are still a few tests missing, mainly the ones for vary headers for multiple and different headers. But you are welcome to give this some testing on real sites.

@pmouawad I think that the caching of content/partial content is a different beast and might be looked at after this pr.

@FSchumacher
Copy link
Contributor Author

I think this feature can be tested now.

@pmouawad
Copy link
Contributor

pmouawad commented Jul 5, 2017

Great @FSchumacher !
Thanks for your work !

@pmouawad
Copy link
Contributor

pmouawad commented Jul 5, 2017

Hi Felix,
Looks good to me.

I'd just change a bit in HTTPJavaImpl#getHeaders(HeaderManager headerManager) to:
private Header[] getHeaders(HeaderManager headerManager) { if (headerManager != null) { final CollectionProperty headers = headerManager.getHeaders(); if (headers != null) { List<Header> allHeaders = new ArrayList<>(headers.size()); for (final JMeterProperty jMeterProperty : headers) { allHeaders.add((Header) jMeterProperty.getObjectValue()); } return allHeaders.toArray(new Header[allHeaders.size()]); } } return new Header[0]; }

@pmouawad
Copy link
Contributor

Hi Felix,
I suggest you commit the PR so that we can update and test code more easily.

Thanks
Regards

asfgit pushed a commit that referenced this pull request Jul 13, 2017
In preparation of Bugzilla Id: 61176 (and github pr #298)


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1801854 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit that referenced this pull request Jul 13, 2017
This should make the code a bit more readable.

In preparation of Bugzilla Id: 61176 (and github pr #298)


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1801855 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit that referenced this pull request Jul 13, 2017
Extract the various methods for calculation of the expiration date into
smaller methods in an attempt to make code more readable.

In preparation of Bugzilla Id: 61176 (and github pr #298)


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1801856 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit that referenced this pull request Jul 13, 2017
In preparation of Bugzilla Id: 61176 (and github pr #298)


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1801857 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit that referenced this pull request Jul 13, 2017
In preparation of Bugzilla Id: 61176 (and github pr #298)


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1801858 13f79535-47bb-0310-9956-ffa450edef68
@asfgit asfgit closed this in aa0169c Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants