chore: Handle too big CH payloads for caching #191

sigua-cs · 2022-07-19T12:19:47Z

Implements #143

render · 2022-07-19T12:19:51Z

Your Render PR Server URL is https://chproxy-pr-191.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cbba2pb0tnuvdk1hnv20.

mga-chka

Hi Guram,
I haven't reviewed your PR yet but you should add a description of this feature in the documentation otherwise it won't be used by chproxy users

render · 2022-07-19T12:56:59Z

Your Render PR Server URL is https://chproxy-pr-191.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cbbak6givq07b7e6n7d0.

mga-chka · 2022-07-20T12:51:08Z

config/config.go

@@ -32,6 +32,8 @@ var (
 	}

 	defaultExecutionTime = Duration(30 * time.Second)
+
+	defaultMaxPayloadSize = ByteSize(100000000)


don't forget in the doc to talk about the default max value

nb: given what @Garnek20 saw in production (for the chatamart cluster), this default size is still to small. IHMO, if we want to have a transparent release (meaning a user that was not using this feature don't understand why some queries are not cached anymore), we should put a very high and unrealistic value like 1<<50.

@sigua-cs Can you add a comment explaining why this value is set so high?

mga-chka · 2022-07-20T12:54:10Z

cache/async_cache.go

@@ -20,6 +20,8 @@ type AsyncCache struct {
 	TransactionRegistry

 	graceTime time.Duration
+
+	MaxPayloadSize int64


since the size must be positive, why not using a uint64 instead of an int64. I'm not sure but I think the ByteSize type is an alias of the uint64 type

Yes, ByteSize is a custom type based on uint64. In the Cache struct maxPayloadSize is a ByteSize, also in the config defaultMaxPayloadSize is a ByteSize. The reason why in the AsyncCache struct it's int64 because we need compare it with the contentLength/bufferedRespWriter which is int64. There is a function which checks if the contentLength not greater than maxPayloadSize, and if the MaxPayloadSize is ByteSize we will need to convert it to int64 every time we check the contentLength

still, I agree with @mga-chka we should not allow building a cache with negative value

mga-chka · 2022-07-20T13:02:56Z

utils.go

+
+func isToCache(length int64, s *scope) bool {
+	maxPayloadSize := s.user.cache.MaxPayloadSize
+	return length <= maxPayloadSize


open question: should we had some metrics in order to know the number of queries not cached because they're too big

sigua-cs · 2022-07-20T16:47:34Z

I need to update the proxy.go according the latest fix version

proxy.go

mga-chka · 2022-08-31T09:13:53Z

proxy.go

 		}
 	} else {
+		// Do not cache responses greater than max payload size.
+		if contentLength > int64(s.user.cache.MaxPayloadSize) {
+			cacheSkipped.With(labels).Inc()


can you add a test that will check that the maxpayloadSize works for a payload bigger and doesn't work for a payload smaller. One way to check it in the test would be to look at the cacheSkipped counter

mga-chka · 2022-09-05T07:29:08Z

main_test.go

@@ -339,6 +374,39 @@ func TestServe(t *testing.T) {
 			},
 			startHTTP,
 		},
+		{
+			"http cache max payload size",


this is a good test but you should also add the test that will asses if when the maxPayloadSize is activated if a payload a smaller than the limit will be cached

gontarzpawel · 2022-09-09T07:32:27Z

main_test.go

+				cachedData, err := cc.Get(key)
+
+				if cachedData != nil || err == nil {
+					t.Fatal("skipped response from cache is expected")


Suggested change

t.Fatal("skipped response from cache is expected")

t.Fatal("response bigger than maxPayloadSize should not be cached")

gontarzpawel · 2022-09-09T07:35:54Z

main_test.go

+				path := fmt.Sprintf("%s/cache/%s", testDir, key.String())
+				if _, err := os.Stat(path); err != nil {
+					t.Fatalf("err while getting file %q info: %s", path, err)
+				}


I don't understand why should we do this check here? IMO it's irrelevant given we're retrieving from cache just after

gontarzpawel · 2022-09-09T07:38:05Z

proxy.go

+		// Do not cache responses greater than max payload size.
+		if contentLength > int64(s.user.cache.MaxPayloadSize) {
+			cacheSkipped.With(labels).Inc()
+			log.Debugf("%s: Request will not be cached. Content length (%d) is greater than max payload size (%d)", s, contentLength, s.user.cache.MaxPayloadSize)


I think it's worth to place it as info log

gontarzpawel · 2022-09-09T07:40:56Z

proxy.go

 		}
 	} else {
+		// Do not cache responses greater than max payload size.
+		if contentLength > int64(s.user.cache.MaxPayloadSize) {


we should complete ongoing transaction even if cache is being skipped

gontarzpawel · 2022-09-09T07:42:07Z

I also don't think distinguishing http vs https is necessary in tests. It's enough to test one given we're interested in checking for if max payload size is exceeded or not @sigua-cs

sigua-cs · 2022-09-09T11:35:11Z

I also don't think distinguishing http vs https is necessary in tests. It's enough to test one given we're interested in checking for if max payload size is exceeded or not @sigua-cs

Removed HTTP tests. Only HTTPS will be used

gontarzpawel

LGTM! Thanks @sigua-cs 🎉

sigua-cs added enhancement good first issue labels Jul 19, 2022

sigua-cs self-assigned this Jul 19, 2022

sigua-cs requested a review from gontarzpawel July 19, 2022 12:19

pull-request-size bot added the size/L label Jul 19, 2022

sigua-cs requested a review from mga-chka July 19, 2022 12:19

sigua-cs force-pushed the chore/cache-max-payload-size branch from d4fb3c0 to 75de0c6 Compare July 19, 2022 12:34

mga-chka reviewed Jul 19, 2022

View reviewed changes

sigua-cs closed this Jul 19, 2022

sigua-cs reopened this Jul 19, 2022

sigua-cs marked this pull request as draft July 19, 2022 12:57

sigua-cs requested review from gontarzpawel and removed request for gontarzpawel July 19, 2022 12:57

sigua-cs force-pushed the chore/cache-max-payload-size branch 2 times, most recently from c6ec7c6 to 98be969 Compare July 19, 2022 13:31

mga-chka reviewed Jul 20, 2022

View reviewed changes

sigua-cs force-pushed the chore/cache-max-payload-size branch 3 times, most recently from 279785d to 509048e Compare July 24, 2022 08:26

sigua-cs commented Jul 24, 2022

View reviewed changes

proxy.go Outdated Show resolved Hide resolved

sigua-cs force-pushed the chore/cache-max-payload-size branch 5 times, most recently from baaeb05 to 867d697 Compare August 8, 2022 06:10

sigua-cs force-pushed the chore/cache-max-payload-size branch from b719487 to 2e43b23 Compare August 21, 2022 07:56

sigua-cs force-pushed the chore/cache-max-payload-size branch from 2e43b23 to c1329fc Compare August 29, 2022 19:44

pull-request-size bot added size/M and removed size/L labels Aug 29, 2022

sigua-cs force-pushed the chore/cache-max-payload-size branch from c1329fc to de46161 Compare August 29, 2022 19:49

pull-request-size bot added size/L size/M and removed size/M size/L labels Aug 29, 2022

mga-chka reviewed Aug 31, 2022

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Sep 4, 2022

mga-chka reviewed Sep 5, 2022

View reviewed changes

sigua-cs force-pushed the chore/cache-max-payload-size branch 3 times, most recently from 3b196e8 to c2a7319 Compare September 7, 2022 15:37

max payload size for caching

c65a989

sigua-cs force-pushed the chore/cache-max-payload-size branch 3 times, most recently from 1829ae8 to 3afaba7 Compare September 7, 2022 19:54

mga-chka approved these changes Sep 8, 2022

View reviewed changes

gontarzpawel requested changes Sep 9, 2022

View reviewed changes

add tests max payload size to proxy_test.go

33755b4

sigua-cs force-pushed the chore/cache-max-payload-size branch from 3afaba7 to 33755b4 Compare September 9, 2022 11:33

sigua-cs requested a review from gontarzpawel September 9, 2022 11:35

gontarzpawel approved these changes Sep 9, 2022

View reviewed changes

mga-chka merged commit d257a95 into master Sep 9, 2022

mga-chka deleted the chore/cache-max-payload-size branch September 9, 2022 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Handle too big CH payloads for caching #191

chore: Handle too big CH payloads for caching #191

sigua-cs commented Jul 19, 2022 •

edited

render bot commented Jul 19, 2022

mga-chka left a comment

render bot commented Jul 19, 2022

mga-chka Jul 20, 2022

mga-chka Jul 20, 2022

gontarzpawel Aug 18, 2022

mga-chka Jul 20, 2022

sigua-cs Jul 24, 2022

gontarzpawel Aug 18, 2022

mga-chka Jul 20, 2022

sigua-cs commented Jul 20, 2022

mga-chka Aug 31, 2022

mga-chka Sep 5, 2022

gontarzpawel Sep 9, 2022

gontarzpawel Sep 9, 2022

sigua-cs Sep 9, 2022

gontarzpawel Sep 9, 2022

sigua-cs Sep 9, 2022

gontarzpawel Sep 9, 2022

sigua-cs Sep 9, 2022

gontarzpawel commented Sep 9, 2022

sigua-cs commented Sep 9, 2022

gontarzpawel left a comment

	t.Fatal("skipped response from cache is expected")
	t.Fatal("response bigger than maxPayloadSize should not be cached")

chore: Handle too big CH payloads for caching #191

chore: Handle too big CH payloads for caching #191

Conversation

sigua-cs commented Jul 19, 2022 • edited

render bot commented Jul 19, 2022

mga-chka left a comment

Choose a reason for hiding this comment

render bot commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigua-cs commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gontarzpawel commented Sep 9, 2022

sigua-cs commented Sep 9, 2022

gontarzpawel left a comment

Choose a reason for hiding this comment

sigua-cs commented Jul 19, 2022 •

edited