-
Notifications
You must be signed in to change notification settings - Fork 504
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
Introduce option to enable dtab compression for Consul #2303
Introduce option to enable dtab compression for Consul #2303
Conversation
93eaba5
to
c675848
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Can you describe what manual testing and verification you did with consul?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I left one comment and as @adleong mentioned would love to know how you tested this. ⭐️ 📦
@@ -63,7 +64,7 @@ case class ConsulDtabInterpreterConfig( | |||
.withParams(tlsParams) | |||
.newService(s"/$$/inet/$serviceHost/$servicePort") | |||
|
|||
KvApi(service, backoffs) | |||
KvApi(service, backoffs, compressValues.getOrElse(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For consistency, we should move this parameter to the section were we do all our val
declarations in this class. I also took a stab at renaming the val as well. WDYT?
diff --git a/namerd/storage/consul/src/main/scala/io/buoyant/namerd/storage/consul/ConsulDtabStoreInitializer.scala b/namerd/storage/consul/src/main/scala/io/buoyant/namerd/storage/consul/ConsulDtabStoreInitializer.scala
index e783ddd4b..72cfa4020 100644
--- a/namerd/storage/consul/src/main/scala/io/buoyant/namerd/storage/consul/ConsulDtabStoreInitializer.scala
+++ b/namerd/storage/consul/src/main/scala/io/buoyant/namerd/storage/consul/ConsulDtabStoreInitializer.scala
@@ -37,6 +37,7 @@ case class ConsulConfig(
val servicePort = port.getOrElse(DefaultPort).port
val backoffs = backoff.map(_.mk).getOrElse(DefaultBackoff)
val tlsParams = tls.map(_.params).getOrElse(Stack.Params.empty)
+ val enableValueCompression = compressValues.getOrElse(false)
val service = Http.client
.interceptInterrupts
@@ -47,7 +48,7 @@ case class ConsulConfig(
.withParams(tlsParams)
.newService(s"/$$/inet/$serviceHost/$servicePort")
new ConsulDtabStore(
- KvApi(service, backoffs, compressValues.getOrElse(false)),
+ KvApi(service, backoffs, enableValueCompression),
root,
datacenter = datacenter,
readConsistency = readConsistencyMode,
linkerd/docs/interpreter.md
Outdated
@@ -189,3 +189,4 @@ writeConsistencyMode | `default` | Select between [Consul API consistency modes] | |||
failFast | `false` | If `false`, disable fail fast and failure accrual for Consul client. Keep it `false` when using a local agent but change it to `true` when talking directly to an HA Consul API. | |||
backoff | exponential backoff from 1ms to 1min | Object that determines which backoff algorithm should be used. See [retry backoff](https://linkerd.io/config/head/linkerd#retry-backoff-parameters) | |||
tls | no tls | Use TLS during connection with Consul. see [Consul Encryption](https://www.consul.io/docs/agent/encryption.html) and [Namer TLS](#namer-tls). | |||
compressValues | `false` | Enables the use of Gzip compression for values stored in Consul. Allows for larger dtabs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding docs!
Thanks for the review. So yes, validating that manually was pretty straightforward. Here are the steps you can take:
This is straight from consul. After you base64 decode the value and decompress it with Gzip,
If there is a better way to verify that let me know. |
Now its important to keep in mind that this is a rather primitive solution in a sense that when you set this compression to match parseContentAs[Entry](rsp.content) {
case Failure(_) => rsp.content
case Success(Entry(data, true)) => decompress(data)
case Success(Entry(data, false)) => data
} This is if we really want to keep it backwards compatible wrt the data already living in a Consul instance. @chrismikehogan maybe it would be useful to share a bit more details around your usecase in order to determine whether this is actually necessary because this try-this-then-that pattern has some perf implications as well. |
c675848
to
7591ae8
Compare
Signed-off-by: Zahari Dichev <[email protected]>
7591ae8
to
32e5d89
Compare
@zaharidichev You raise some really good points. I haven't heard back from anyone who had been requesting this feature. I think we should put this aside for now (perhaps close the PR but keep the branch) until we get more clarity on the use-case. This implementation is solid but I'd rather not merge a feature that isn't actually useful to anyone. Does that make sense? |
Yes, certainly makes sense. Lets avoid codebase bloating. If we get more clarity on the request, I will pick that up again. |
Closing for now due to lack of clarity on requirements. |
Allows for making use of Gzip compression when interacting with Consul KV Api.
Fixes #2202
Signed-off-by: Zahari Dichev [email protected]