search

twosigma / satellite

Satellite monitors, alerts on, and self-heals your Mesos cluster.
Apache License 2.0
143 stars 18 forks source link

For review: Static configuration #54

Closed mforsyth closed 8 years ago

mforsyth commented 8 years ago

Attn @wkf I will add documentation once the code itself is all approved...

wkf commented 8 years ago

Just a small comment, otherwise LGTM.

wyegelwel commented 8 years ago

Just checking, The intention is first to add static config support and then add support for no-clojure configuration?

mforsyth commented 8 years ago

@wyegelwel Actually, the intention here is to have the clojure config file and the data file it loads co-exist, & see if that makes people happy. My thinking: the problem isn't that a clojure file exists outside the jar, it's that you have to edit clojure code in order to do things like changing thresholds and hostnames, and adding or removing predefined comets.

There are advantages to "coexistence" rather than asking users to pick either .clj or .json. It means that the decision isn't very significant - if someone starts out just wanting to do things like changing the slave to report every 30 seconds instead of every 60 seconds, he can just edit JSON. Later on, if he realizes that he needs to write a custom comet, he doesn't have to change much; he can leave his custom JSON file where it is, and edit the .clj file to add the custom comet.

mforsyth commented 8 years ago

OK @tnn1t1s @wyegelwel I've made changes based on your comments. The slave's .json file is way more self-documenting now (and flexible!). Thanks :)

wyegelwel commented 8 years ago

lgtm. What do you think @tnn1t1s?

tnn1t1s commented 8 years ago

I'm good

On Fri, Feb 12, 2016 at 11:13 AM wyegelwel notifications@github.com wrote:

lgtm. What do you think @tnn1t1s https://github.com/tnn1t1s?

— Reply to this email directly or view it on GitHub https://github.com/twosigma/satellite/pull/54#issuecomment-183391139.

mforsyth commented 8 years ago

Alright, thanks guys, I will just add some documentation and then we'll get this merged.