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

Proposal for combining some attributes #45

Closed
jakedavis opened this issue Jul 9, 2014 · 17 comments
Closed

Proposal for combining some attributes #45

jakedavis opened this issue Jul 9, 2014 · 17 comments

Comments

@jakedavis
Copy link
Contributor

Howdy. We're starting to play with this cookbook and I'd like to propose renaming a few attributes to make it a bit easier to combine like-parameters. I was thinking something like,

  • node[:kafka][:broker]
  • node[:kafka][:topic]
  • node[:kafka][:consumer]
  • node[:kafka][:producer]

This opens the door for replacing templates with methods which just render the config correctly. It's my first time deploying Kafka, so the above organization might not make complete sense. Something like node[:kafka][:config] under which everything else is buried might be good, alternatively.

Anyway, would be willing to put in the time to PR this, but wanted to check if this is something you're interested in first.

@dpkp
Copy link
Contributor

dpkp commented Jul 10, 2014

I'm confused as to what you are proposing. what would you propose as default values? maybe that would help me understand your thinking here.

@jakedavis
Copy link
Contributor Author

Ah sorry, so each of those would be a hash pointing to the same sorts of config items as now. Not to plug our own stuff, but something like this as an example. Mostly, just to consolidate the configuration parameters into a separate, isolated namespace, which lets you do things like render the config file without a template, etc.

The default values of the parameters would remain the same, it's just reorganizing how they're namespaced. Sorry for the confusion!

@dpkp
Copy link
Contributor

dpkp commented Jul 10, 2014

ah got it -- so you are trying to get consumer and producer library configs? I've only ever used this cookbook to setup brokers.

@jakedavis
Copy link
Contributor Author

Oh, right. In that case, might be worth just having node[:kafka][:config].

@mthssdrbrg
Copy link
Contributor

Heya,

The attributes that are currently being used for rendering the main configuration file attempts to follow some kind of naming convention, mapping to the actual configuration keys used by Kafka, i.e. attributes that are in the socket "namespace" all start with socket. in the actual configuration file. Unfortunately the Kafka team have been quite inconsistent when naming configuration options, and they tend to add/remove configuration options between "patch" releases (which is a pain to attempt to maintain...), though I don't think they follow semantic versioning to any degree when it comes to configuration at least.

I've been playing with the idea of removing the current attempt at grouping options and just go with a completely flat namespace, and with this in mind I can see a point in grouping everything under a config namespace, even though I haven't seen it used in many other cookbooks (it's been a while though). But really one could explicitly pass all configuration options using variables on the template resource.

What I'd really like is a solution that makes it "easy" to manage the rendering of configuration files between different releases, for example one "renderer" for 0.8.0, one for 0.8.1, etc, etc. That would also imply an easy way to manage configuration options between releases, and I'm not sure how to go about that. While one would always like to be using the latest and "bestest" version of software, it's not always possible to due a number of reasons that vary between companies and cultures.

In short I'd like to make something smart when it comes to configuration file rendering, but I'm not sure how to approach the issue. But I don't think putting all configuration options under a config namespace is going to make it much better, albeit it might make it a bit easier to test (maybe).

I'd like to ponder this a bit and try out some things. If you have suggestions or opinions on how you'd like things to work (preferably with concrete examples) that'd be much appreciated! Sorry about the length of this comment, but I've had this on my mind back and forth a while.

@mthssdrbrg
Copy link
Contributor

By the way, I should probably mention that I've yet to actually deploy a Kafka cluster using this cookbook :). Should've done it to our staging environment this week, but looks like it might have to wait until next week.

@jakedavis
Copy link
Contributor Author

That's one of the nice things about the flat pattern, is you can specify just the defaults, and then avoid further levels of namespacing. In that case, you could replace periods with, e.g., underscores and implement logic in the rendering method to replace them when it runs. So like,

node[:kafka][:config] = {
  log_dirs: ['/mnt/kafka']
}

node[:kafka][:config].collect do |k,v|
  "#{k.gsub('_', '.')}=#{v}"
end

Or what not.

The config versioning thing is interesting. Usually my feeling on that is to specify as little as possible, since Kafka provides defaults for non-mandatory parameters. In that case, if the mandatory parameters change, you're still in trouble, but otherwise the user of this cookbook is expected to know the changes they're bringing onboard. So if queued.max.requests changed to queued.maximum.requests, presumably I've upgraded my Kafka version, which shifts the onus on me to change that attribute, since this cookbook didn't name it specifically, but provided an easy/forwards-compatible way to insert or delete parameters.

@mthssdrbrg
Copy link
Contributor

Yeah, that's definitely a big win with having everything scoped under a config namespace, though one has to do some special logic when it comes to Arrays and whatnot, but that's definitely doable (it's Ruby after all :)).
Initial feels about this is that it would make it a lot more maintainable when it comes to different configuration options between releases as it would put the burden on the user of the cookbook rather than the maintainer (as it should be, in my opinion). Then there could be specialized unit tests for different versions, etc, etc.

From the start I defined all the current defaults (according to the Kafka documentation), but later switched to just explicitly name all the configuration options in attributes/default.rb with either a nil value or an empty Array (where Arrays are appropriate).

@mthssdrbrg
Copy link
Contributor

I've fleshed out a first stab at dynamically generating the main configuration file in #47, and I must say that it looks pretty promising.

Everything (i.e. configuration attributes) is currently scoped under a broker namespace, as config was already used for the filename of the main configuration file (might change that though). What do you guys think?

@mthssdrbrg
Copy link
Contributor

So, I just merged #47 and now all configuration attributes should be defined under the broker namespace. And I just realized that I should probably update the README to reflect these changes, or at least document it in the default attributes file. Oh well.

@jakedavis
Copy link
Contributor Author

@mthssdrbrg, thanks a ton!

@mthssdrbrg
Copy link
Contributor

No probs. I'm gonna hold off on releasing a new version for a little bit though, got some other bits and pieces that I want to incorporate before the next release.

@jakedavis
Copy link
Contributor Author

That's okay, we use git refs ;)

@mthssdrbrg
Copy link
Contributor

Cool. If you find anything weird while testing it out let me know / open a pull request ;). There might've been some breaking changes that I haven't considered yet, so any feedback is welcome.

@rberger
Copy link

rberger commented Oct 20, 2014

Did you guys end up doing anything for configuring the consumer.properties file? I can't seem to find anything in the cookbook for that. I presume right now its just the default from the package install?

I have deployed a 3 node kafka / 3node zookeeper with the cookbook (though I haven't passed data thru it yet :-) Hopefully tonite!

@mthssdrbrg
Copy link
Contributor

No, as the intention of this cookbook is to set up a Kafka broker and not for client applications. That being said, you could use the Configuration in a similar way to what's done here, but that might just be a bit overkill :)

@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants