Skip to content
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

Closed

Conversation

zaharidichev
Copy link
Member

Allows for making use of Gzip compression when interacting with Consul KV Api.

Fixes #2202

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

@zaharidichev zaharidichev changed the title Introduces option to enable dtab compression for Consul Introduce option to enable dtab compression for Consul Jul 26, 2019
Copy link
Member

@adleong adleong left a 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?

Copy link
Contributor

@dadjeibaah dadjeibaah left a 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))
Copy link
Contributor

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,

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding docs!

@zaharidichev
Copy link
Member Author

zaharidichev commented Jul 27, 2019

@adleong @dadjeibaah

Thanks for the review. So yes, validating that manually was pretty straightforward. Here are the steps you can take:

  • Fire up consul: ./consul agent -dev -advertise 127.0.0.1
  • Start Namerd:
admin:
  ip: 0.0.0.0
  port: 9991
interfaces:
  - kind: io.l5d.httpController
storage:
  kind: io.l5d.consul
  compressValues: true
  • Create a dtab entry via the API:
curl -X POST \
  http://127.0.0.1:4180/api/1/dtabs/graduation \
  -H 'Content-Type: application/json' \
  -H 'Host: 127.0.0.1:4180' \
  -d '[{"prefix":"/yeezy","dst":"/kanye"}]'
  • Do a curl http://127.0.0.1:8500/v1/kv/namerd/dtabs/graduation and you will get something like this:
[
    {
        "LockIndex": 0,
        "Key": "namerd/dtabs/graduation",
        "Flags": 0,
        "Value": "SDRzSUFBQUFBQUFBQU5PdlRFMnRxclMxMDg5T3pLdE1CUUFBeWttbkRnQUFBQT09",
        "CreateIndex": 16,
        "ModifyIndex": 16
    }
]

This is straight from consul. After you base64 decode the value and decompress it with Gzip,
you can convince yourself that indeed Consul is stroing the gzipped representation of /yeezy=>/kanye

[
    {
        "prefix": "/yeezy",
        "dst": "/kanye"
    }
]

If there is a better way to verify that let me know.

@zaharidichev
Copy link
Member Author

Now its important to keep in mind that this is a rather primitive solution in a sense that when you set this compression to true, you assume that your consul instance will only contain compressed dtabs, so you cannot really go back. So if one wants to transition to this approach while keeping their old data in consul, its not possible.. Instead what we would have to do it maybe wrap the data we store in consul in some wrapper such as case class Entry(data: String, compressed: Boolean)
So then the logic (in order to be able to support values written to consul before this change) would be something like:

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.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@adleong
Copy link
Member

adleong commented Aug 15, 2019

@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?

@zaharidichev
Copy link
Member Author

Yes, certainly makes sense. Lets avoid codebase bloating. If we get more clarity on the request, I will pick that up again.

@adleong
Copy link
Member

adleong commented Aug 20, 2019

Closing for now due to lack of clarity on requirements.

@adleong adleong closed this Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for compressed dtabs in Consul
3 participants