Skip to content
This repository has been archived by the owner on Apr 23, 2020. It is now read-only.

Add support for consul shared configuration #305

Merged
merged 2 commits into from Jul 29, 2017

Conversation

thesquelched
Copy link
Contributor

Add consul as a shared config provider. The relevant CLI options are:

 -c,--configtype <arg>         Defines which configuration type you want
                               to use. Choices are: "file", "s3",
                               "zookeeper", "consul" or "none". Additional
                               config will be required depending on which
                               type you are using.

== Configuration Options for Type "consul" ==
    --consulacltoken <arg>            Optional Consul ACL token
    --consulhost <arg>                Consul host; defaults to "localhost"
    --consulport <arg>                Consul HTTP(s) port; defaults to
                                      8500
    --consulprefix <arg>              Prefix in the key-value store under
                                      which to store Exhibitor data, e.g.
                                      "exhibitor/"
    --consulssl <arg>                 If true, enables Consul
                                      communication over SSL
    --consulsslcacert <arg>           Path to the consul CA cert file
    --consulsslprotocol <arg>         Consul SSL/TLS protocol; defaults to
                                      "TLSv1.2"
    --consulsslverifyhostname <arg>   If true, verify SSL hostnames

Fixes #280

@Randgalt
Copy link
Contributor

FYI - I'm in discussion with Netflix to move Exhibitor to its own repo so that it get better community support. Please stayed tuned.

@thesquelched
Copy link
Contributor Author

Step 1: Fork
Step 2: ???
Step 3: Profit?

@supernovae
Copy link
Contributor

Would love to see this mainline, but as @thesquelched said - fork!

@supernovae
Copy link
Contributor

Successfully using this on a dev environment. No issues reported so far.

@scalp42
Copy link

scalp42 commented Mar 7, 2017

Any chance to see this merged ?

@thesquelched
Copy link
Contributor Author

I very much doubt it

@flyinprogrammer
Copy link

@supernovae and I have this code running on 22 mesos clusters and counting with this additional patch: https://github.com/flyinprogrammer/exhibitor/commit/d1cd3c1d30b1d5dba405c380c1fa7e1dec70ab1f

for better or worse lol -- thanks @thesquelched for writing this, you're saving our 🥓 tomorrow night!

@thesquelched
Copy link
Contributor Author

thesquelched commented Apr 12, 2017

I'll try to update the PR today with that @flyinprogrammer

@nickwales
Copy link

We (@supernovae & @flyinprogrammer) are collecting thousands of sessions currently, can deletion of redundant sessions be implemented?

@thesquelched
Copy link
Contributor Author

@flyinprogrammer updated to include your library updates (and to rebase to remove merge conflicts).

@nickwales can you give me any more details for the sessions issue?

@nickwales
Copy link

@thesquelched sure, we're monitoring the telemetry data and there was a bump in session creation without a similar jump in session deletion at the time we started using the consul backend.

Image of how it looks in datadog currently: http://imgur.com/a/ti35J

One of our clusters currently has 31818 sessions, 31145 of those have the name 'exhibitor'.

Let me know if there's anything else you need!

@thesquelched
Copy link
Contributor Author

@nickwales yeah, I'm an idiot. I never bothered to destroy the sessions after releasing locks. I've added that, as well as adding a TTL on sessions so that they don't last forever if something goes wrong. Let me know if it works for you.

@thesquelched
Copy link
Contributor Author

Oh, and I confirmed that my tests (without destroying sessions) leave sessions laying around after changing the config, so that's almost certainly the issue. I modified the tests to check that sessions aren't left laying around anymore.

@nickwales
Copy link

Awesome, I'll get @flyinprogrammer to upgrade immediately 🥇

@xiaochuanyu xiaochuanyu merged commit c919b3c into soabase:master Jul 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support consul as shared config provider
7 participants