Edge cache integration #231
Conversation
There was a problem hiding this comment.
👍 overall. Still not sure on making the config more generic. There are some tricky jackson things we could probably do to have separate classes and serialize as a different one depending on the value of that enum field. Alternatively we could have an edge cache associated with a specific baragon group (since they are generally each associated with a domain anyways).
| */ | ||
| @Override | ||
| public boolean invalidateIfNecessary(BaragonRequest request) { | ||
| if (request.getLoadBalancerService().getOptions().get("edgeCacheDNS") == null) { |
There was a problem hiding this comment.
edgeCacheDNS as a static variable since it's used a few times?
| } | ||
|
|
||
| private Optional<CloudflareZone> getCloudflareZone(String edgeCacheDNS) throws CloudflareClientException { | ||
| List<CloudflareZone> zones = cf.getAllZones(); |
There was a problem hiding this comment.
how expensive is this call? Should this be cached at all?
|
|
||
| // The following three values are used by the Cloudflare EdgeCache implementation. | ||
| // TODO: It's kind of a smell to have implementation-specific configs in the generic EdgeCacheConfiguration block. | ||
| // TODO: Is there a better way of doing this? |
There was a problem hiding this comment.
I think these fields are generic enough that they would work for more cases. Still thinking on ways we could make it more generic/reusable. I'll comment if I come up with something
There was a problem hiding this comment.
Hm, I'm not familiar with how Dropwizard YAML -> Config Class magic works specifically, but can we just have a Map<String, String> options in the EdgeCacheConfiguration class, and then document the specific supported options for each EdgeCache implementation?
| } | ||
|
|
||
| try { | ||
| String edgeCacheDNS = ((String) request.getLoadBalancerService().getOptions().get("edgeCacheDNS")); |
There was a problem hiding this comment.
would it make more sense to make this an explicit field on the BaragonService instead of having it be something in the options? The options have generally been used with the purpose of injecting data into the templates to be rendered, which is why it is a <String, Object> instead of something more specific. If we have an optional field on the baragon service, it's then a much cleaner call like service.getEdgeCacheDNS.isPresent().
Also, I could see a case where an app is hosted on multiple domains (multiple Baragon groups) and might need multiple things cleared in which case it'd need to be more of a list of domains. Do we expect to need to handle that use case?
|
|
||
| return cf.purgeCache( | ||
| zoneId, | ||
| Collections.singletonList( // TODO this isn't the right way to get the env, is it vvv |
There was a problem hiding this comment.
Baragon doesn't have a concept of the internal HubSpot env. Would it make more sense to either:
- have the list of cache tag formats provided on the BaragonService
or
- configure the list of cache tag formats to be cleared in the config yaml
No description provided.