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

Closes #30. Add the ability to specify custom NameNode name and edits dirs. #40

Merged
merged 1 commit into from
May 7, 2018

Conversation

xkrogen
Copy link
Collaborator

@xkrogen xkrogen commented May 4, 2018

No description provided.

@@ -59,7 +63,7 @@
AMOptions(int datanodeMemoryMB, int datanodeVirtualCores, String datanodeArgs,
String datanodeNodeLabelExpression, int datanodesPerCluster, String datanodeLaunchDelay, int namenodeMemoryMB,
int namenodeVirtualCores, String namenodeArgs, String namenodeNodeLabelExpression, int namenodeMetricsPeriod,
Map<String, String> shellEnv) {
String namenodeNameDir, String namenodeEditsDir, Map<String, String> shellEnv) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor is growing huge and a lot have default values, shall we construct this through a builder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought was to agree with you, but actually, I think it makes sense for it to stay as-is. Since this is just an internal class for encapsulating the result of a number of command-line options, you wouldn't normally call this constructor directly, and in fact the only current usage is within this class (initFromParser()). It does not seem worth creating a builder given this setup.

@xkrogen xkrogen merged commit bfac897 into linkedin:master May 7, 2018
@xkrogen xkrogen deleted the ekrogen-name-edits-dir branch May 7, 2018 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants