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

add support for hdfs integration #77

Merged
merged 2 commits into from Nov 27, 2013

Conversation

Projects
None yet
2 participants
@phlipper
Copy link
Contributor

phlipper commented Nov 23, 2013

Add support for the hdfs integration to the chef cookbook.

metadata.rb Outdated
@@ -19,6 +19,7 @@

depends "apt" # We recommend '>= 2.1.0'. See CHANGELOG.md for details
depends "chef_handler", "~> 1.1.0"
depends "python"

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

@phlipper Introducing another cookbook dependency is a little heavy-handed for most users - they may have other ways of installing python packages, and may not want the default behaviors included.

I would add a suggests clause for this, and not force it.

This comment has been minimized.

Copy link
@phlipper

phlipper Nov 25, 2013

Author Contributor

@miketheman I didn't realize there were other ways to get the python_pip resource. suggests is a better way here, I'll update that shortly.

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

@phlipper To be clear - suggests doesn't actually change the behavior of the cookbook, rather leaves it up to the operator to make a decision.

include_recipe "datadog::dd-agent"

package "python-pip"
package "python-setuptools"

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

These two packages would be handled by the python cookbook, so to get them you would use include_recipe 'python' in this recipe, which would also bring in the python_pip resource.

This comment has been minimized.

Copy link
@phlipper

phlipper Nov 25, 2013

Author Contributor

Take makes sense, I'll update it.

# Port defaults to 8020.
instances:
<% @instances.each do |instance| -%>
- namenode: <%= instance.fetch("fqdn"){ node["fqdn"] } %>

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

I don't think I've seen this pattern before - can you elaborate on why this is better/different?

This comment has been minimized.

Copy link
@phlipper

phlipper Nov 25, 2013

Author Contributor

I like Hash#fetch because I feel that it's a little more intention-revealing than the more commonly used hash[hey] || other pattern. This isn't a case that's exceptional enough to raise so the x || y pattern is fine here, but I just like fetch these days :) I'd be happy to change it if you'd like.

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

I guess what I'm unclear on is what the structure of instances is meant to be here - a hostname, and fqdn, and IP, node object, etc? Can you provide a sample hash in a comment in the recipe, similar to the apache recipe?

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Nov 25, 2013

@phlipper Awesome looking stuff - I had a few questions about this particular PR.

changes per review:
* `supports` python vs. `depends`
* `include_recipe "python"` vs. install packages
* add comments for hdfs instances format
@phlipper

This comment has been minimized.

Copy link
Contributor Author

phlipper commented Nov 25, 2013

@miketheman I have made updates based on your comments. How does it look now?

@@ -20,6 +20,7 @@
depends "apt" # We recommend '>= 2.1.0'. See CHANGELOG.md for details
depends "chef_handler", "~> 1.1.0"
depends "yum"
supports "python"

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

I think this was meant to be 'suggests' - 'supports' typically denotes the platform it's meant to run on.

#
# node["datadog"]["hdfs"]["instances"] = [
# {
# "fqdn" => "hdfs.prod.tld",

This comment has been minimized.

Copy link
@miketheman

miketheman Nov 25, 2013

Collaborator

So I guess I'm still unsure on this part. I'm guessing this integration ought to run either on your NameNode, or on a single node that can access your NameNode.

If the fqdn attribute not set, then the template .fetch mechanism in the template will default to the node['fqdn'] attribute?

miketheman added a commit that referenced this pull request Nov 27, 2013

ignore the FC rule for this particular condition
Since we wish to allow users to make the best choices, leaving some of
the external Chef requirements up to the user to handle instead of
mandating that they use a particular version of a dependency cookbook.

This was further discussed in #77

@miketheman miketheman merged commit 9f39ec8 into DataDog:master Nov 27, 2013

1 check failed

default The Travis CI build failed
Details
@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Nov 27, 2013

Hi @phlipper!

I went ahead and fixed the build issue, the other comments I had, etc.

Thanks for your time!
-M

@ghost ghost assigned miketheman Nov 27, 2013

@phlipper

This comment has been minimized.

Copy link
Contributor Author

phlipper commented Nov 27, 2013

@miketheman thanks for taking the time to do the last few bits of cleanup, and for getting this merged!

@phlipper phlipper deleted the Shopify:hdfs branch Nov 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.