Only batch fetch when there is a sufficiently large number requested #316
Conversation
dturn
force-pushed
the
bypass-batched-fetching
branch
from
e5bf00d
to
d6e0014
Compare
|
PR that added batch fetching #251. Doesn't look like the tests had major changes. When When set to
|
dturn
force-pushed
the
bypass-batched-fetching
branch
from
d39a44a
to
8436cab
Compare
dturn
force-pushed
the
bypass-batched-fetching
branch
from
8436cab
to
9752189
Compare
|
@Shopify/cloudx this is ready for 👀 |
| @@ -1,6 +1,8 @@ | |||
| # frozen_string_literal: true | |||
| module KubernetesDeploy | |||
| class SyncMediator | |||
| LARGE_BATCH_THRESHOLD = 5 | |||
There was a problem hiding this comment.
What makes you choose 5? I would expect a much higher number. We fetch 8-way parallel, so I'd be inclined to define the threshold in relation to how many batches of concurrent requests it will take... maybe 3?
On the other hand, what do you think about applying the threshold per class rather than globally? E.g. maybe you have 15 things, but they're each of a different kind, so the batched fetching doesn't actually help. I'm thinking it would be more accurate in terms of using the batch strategy when it actually helps, but it would need to take sync dependencies into account and definitely be more complicated.
There was a problem hiding this comment.
5 was a number out of a hat, I didn't want to get bogged down with this number before seeing how the code looked. I think 3 x parallelism makes sense.
I agree we could get a lot more sophisticated here, but it seems like you're a small app who wont batch or your giant and the extra logic wont make a difference in most cases.
test/unit/sync_mediator_test.rb
Outdated
|
|
||
| def test_sync_does_not_batch_with_few_resources | ||
| stub_kubectl_response('get', 'FakeConfigMap', @fake_cm.name, *@params, | ||
| resp: { "items" => [@fake_cm.kubectl_response] }, times: 0) |
There was a problem hiding this comment.
times: 0 seems to still expect a request, but not return a valid response from it... why? I'd think we'd want a .expects.never here on a list call (the one here is an instance call for @fake_cm), and have an actual stub on the instance API call we expect rather than the expects below, to show what happened to the cache (it wasn't populated).
test/unit/sync_mediator_test.rb
Outdated
| mediator.sync(test_resources) | ||
| end | ||
|
|
||
| def test_sync_does_batch_with_enough_resources |
There was a problem hiding this comment.
What value does this add beyond test_sync_caches_the_types_of_the_dependencies_of_the_instances_given and test_sync_caches_the_types_of_the_instances_given (which should probably have their names amended)? Because this test is stubbing the instance syncs, it isn't actually proving anything about the cache, just that the list api call was made (which those existing tests also do).
test/unit/sync_mediator_test.rb
Outdated
| mediator.sync(test_resources) | ||
| end | ||
|
|
||
| def test_sync_does_not_batch_with_few_resources |
There was a problem hiding this comment.
nit: I know I talked about this in terms of batching, but the real behaviour difference might be better described in terms of whether or not sync warms the type cache
There was a problem hiding this comment.
That makes a lot more sense.
test/unit/sync_mediator_test.rb
Outdated
| end | ||
|
|
||
| def test_sync_does_not_warm_cache_with_few_resources | ||
| KubernetesDeploy::Kubectl.any_instance.expects(:run).with('get', 'FakeConfigMap', @fake_cm.name, *@params).never |
There was a problem hiding this comment.
I guess it doesn't really matter because an exception will be raised if either request is attempted, but technically the cache warming would be requesting the list, not the instance.
test/unit/sync_mediator_test.rb
Outdated
|
|
||
| test_resources = [@fake_pod, @fake_cm, @fake_cm2, @fake_deployment] | ||
| test_resources = [@fake_cm] |
There was a problem hiding this comment.
Let's make this test the relevant edge by multiplying it by KubernetesDeploy::SyncMediator::LARGE_BATCH_THRESHOLD
test/unit/sync_mediator_test.rb
Outdated
|
|
||
| test_resources = [@fake_pod, @fake_cm, @fake_cm2, @fake_deployment] | ||
| test_resources = [@fake_cm] | ||
| test_resources.each { |r| r.expects(:sync).once } | ||
| mediator.sync(test_resources) |
There was a problem hiding this comment.
For additional proof, we could add a request stub after this and do mediator.get_instance('FakeConfigMap', @fake_cm.name) to show the cache is not warm (kinda the opposite of what the cache-is-warm tests above do).
|
ping |
Polling can take a long time for small resource groups when the cluster contains a large number of resources of that type. Don't batch fetch when number of resources being tracked is small.
fixes: #314