Skip to content
This repository has been archived by the owner on Jan 8, 2019. It is now read-only.

Zookeeper updates #1233

Merged
merged 13 commits into from
Jul 12, 2018
Merged

Zookeeper updates #1233

merged 13 commits into from
Jul 12, 2018

Conversation

cbaenziger
Copy link
Member

This addresses the concerns of #1214 and #52 as well it brings in locking_resource for the Zookeeper servers themselves.

@@ -32,5 +36,16 @@ SERVER_JVMFLAGS="$SERVER_JVMFLAGS -Djava.security.auth.login.config=/etc/zookeep
CLIENT_JVMFLAGS="$CLIENT_JVMFLAGS -Djava.security.auth.login.config=/etc/zookeeper/conf/zookeeper-client.jaas"
<% end %>

<%
auto_size = (node['memory']['total'].to_i *
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move calculations outside tempates?

-%>
SERVER_JVMFLAGS="$SERVER_JVMFLAGS -Xms<%= heap %>m -Xmx<%= heap %>m " \
"-Xmn<%= newsize %>m " \
"<%= node['bcpc']['hadoop']['zookeeper']['gc_opts'] %>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

FC0035 is deprecated but I think this template has enough access to various parts of the node Object that it is worth enforcing.

See Foodcritic/foodcritic#62 for background. Enforcing it is getting too draconian but it still makes sense to enforce in the other extreme.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aespinosa I am not quite sure what direction you are wanting this to go in but at this time a change for node object access here should be consistent with the other -env.sh files we have and would be a broader scope of work than I am comfortable with for this change. For reference, we have the following if you want to file an issue to tackle this?

@cbaenziger
Copy link
Member Author

The original built a working cluster. Rebuilding to ensure post code-review changes also work.

@cbaenziger
Copy link
Member Author

It looks like cluster-assign-roles.sh ran through after some unrelated network issues running behind proxies.

@aespinosa aespinosa merged commit e80fd28 into bloomberg:master Jul 12, 2018
@aespinosa
Copy link
Collaborator

@cbaenziger this doesn't backport cleanly.

Mind sending a separate PR to the release-3.3 branch?

@cbaenziger cbaenziger deleted the zookeeper_updates branch July 16, 2018 13:37
@aespinosa aespinosa mentioned this pull request Jul 27, 2018
@aespinosa aespinosa mentioned this pull request Jul 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants