Vary header cache by FSchumacher · Pull Request #298 · apache/jmeter

FSchumacher · 2017-06-19T18:36:58Z

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.

undera

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.

undera · 2017-06-19T19:03:16Z

src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java

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


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

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

undera · 2017-06-19T19:05:47Z

src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPJavaImpl.java

        }
    }

+    private Header[] getHeaders(HeaderManager headerManager) {


I'd make this protected for extensibility

I thought of extracting it into a utility class.

vlsi

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

vlsi · 2017-06-20T08:06:32Z

src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java

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


Please move string concatenation out of a loop

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.

vlsi · 2017-06-20T08:07:38Z

src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java

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


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

…e, only

…ched.

This depends on HashMap#toString to give consistent results, which is probably not true.

pmouawad · 2017-06-27T19:42:55Z

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

https://bz.apache.org/bugzilla/show_bug.cgi?id=53540

Thanks

…ders.

undera · 2017-06-29T21:23:38Z

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

FSchumacher · 2017-07-01T16:53:32Z

@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 · 2017-07-05T19:55:21Z

I think this feature can be tested now.

pmouawad · 2017-07-05T20:00:06Z

Great @FSchumacher !
Thanks for your work !

pmouawad · 2017-07-05T20:29:08Z

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 · 2017-07-12T21:02:09Z

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

Thanks
Regards

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

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

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

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

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

FSchumacher added 6 commits June 19, 2017 20:27

Let the logging facade decide, whether we are in the correct logging …

d9c5a69

…mode. We are not not doing any real work here and it simplifies our code.

Use log string templates instead of string concatenation.

b0d306c

This should make the code a bit more readable.

Extract code into local private methods.

fe4c0fb

Extract the various methods for calculation of the expiration date into smaller methods in an attempt to make code more readable.

Rename local variable to comply with java naming conventions.

6bb6e8e

Reformat to make it easier to read.

fb661df

Use vary header information for caching.

f38a460

undera reviewed Jun 19, 2017

View reviewed changes

vlsi requested changes Jun 20, 2017

View reviewed changes

FSchumacher added 5 commits June 20, 2017 19:26

Guard debug log message with a computation, to be called in debug mod…

fed1738

…e, only

Remove unused imports.

726b08d

A vary header with '*' as value means, that the page should not be ca…

2263691

…ched.

First try to support multiple vary headers.

4ff79dc

This depends on HashMap#toString to give consistent results, which is probably not true.

Don't loose expire date on calculation

7c81a4b

FSchumacher added 2 commits June 29, 2017 20:06

Tests for vary header are running, except those for multiple vary hea…

d4d1500

…ders.

Tests for multi value vary header pass.

5a24ebc

FSchumacher added 2 commits July 5, 2017 21:08

Correct tests for multiple vary headers

117fafc

Remove mentioning of the not-caching of Vary headers.

3e7425b

asfgit closed this in aa0169c Jul 13, 2017

Conversation

FSchumacher commented Jun 19, 2017

Uh oh!

undera left a comment

Choose a reason for hiding this comment

Uh oh!

undera Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

FSchumacher Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

undera Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

FSchumacher Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

vlsi left a comment

Choose a reason for hiding this comment

Uh oh!

vlsi Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

FSchumacher Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

vlsi Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

pmouawad commented Jun 27, 2017

Uh oh!

undera commented Jun 29, 2017

Uh oh!

FSchumacher commented Jul 1, 2017

Uh oh!

FSchumacher commented Jul 5, 2017

Uh oh!

pmouawad commented Jul 5, 2017

Uh oh!

pmouawad commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmouawad commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pmouawad commented Jul 5, 2017 •

edited

Loading