Crozzy auto config #41
Conversation
|
also, feel free to reject #38 |
| "strings" | ||
| ) | ||
|
|
||
| type Property struct { |
There was a problem hiding this comment.
This and Result don't need to be capitalized, since they aren't exported. Also I would name Result something like propertyList.
|
Ok - left a bazillion comments, sorry =) Thanks again for your work on this! I'll close #38. |
|
|
||
| type HadoopConf map[string]string | ||
|
|
||
| var ErrUnresolvedNamenode = errors.New("Couldn't find a namenode address in any config.") |
There was a problem hiding this comment.
Go errors should be shorthand, uncapitalized and unpunctuated, like "no namenode address in config".
|
No problem! Let know if there are some more changes for this PR. |
| if nnErr != nil { | ||
| return "", nnErr | ||
| } | ||
| return namenodes[len(namenodes)-1], nil |
There was a problem hiding this comment.
Maybe this should be namenodes[0]
There was a problem hiding this comment.
Yeah I guess that's slightly better (but if we have multiple I'm not really sure what the behavior should be, outside of HA support).
| if hadoopHome != "" { | ||
| hdfsPath := path.Join(hadoopHome, "conf", "hdfs-site.xml") | ||
| corePath := path.Join(hadoopHome, "conf", "core-site.xml") | ||
| tryPaths = append(tryPaths, []string{hdfsPath, corePath}...) |
There was a problem hiding this comment.
this can just be append(tryPaths, confHdfsPath, confCorePath) (and the same below)
|
Ok, looks awesome. A couple more minor changes and then I'll be happy to merge! |
|
Sweet! Thanks for the tips. |
|
Yay! Thank you so much for the PR. |
Hey @colinmarc , I think this is a little cleaner. Let me know if you have some other suggestions.