Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Use class variable to avoid creating new connection for each resource #205

Merged
merged 1 commit into from
Oct 19, 2017

Conversation

GolubevV
Copy link
Contributor

In existing implementation, the zookeeper connection is cached in the class instance variable as part of the ZK::Gem module method, which is later mixed-in to the zookeeper_node custom resource class.
As a consequence, new ZK connection is created for each instance of the zookeeper node resource. As zookeeper has 60 connections allowed from single IP by default to protect from the DOS-attack, it means that total number of zookeeper node resources in a single run is also limited to 60 due to creation of new connection per each resource.
As a possible quick fix, I propose to use the class variable, allowing to share a connection object between all resource objects.

Copy link
Contributor

@jeffbyrnes jeffbyrnes left a comment

Choose a reason for hiding this comment

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

Seems like a rare, sensible use of a class-level variable.

@jeffbyrnes jeffbyrnes merged commit 6af1f32 into evertrue:master Oct 19, 2017
@GolubevV
Copy link
Contributor Author

Thanks for reviewing and merging! The only potential problem with it that I think of - is when somebody is managing multiple different Zookeeper instances from single chef run_list. In that case, the old cached connection might be reused for new resource with potentially different connection string.
This potential issue may be fixed for example by having a class variable for a hash of { connection_string => connection_object } keypairs and check for each new connection string to be present in hash.
Do you think this is a viable use case that should be addressed?

@jeffbyrnes
Copy link
Contributor

jeffbyrnes commented Oct 23, 2017

While I’m sure somebody is managing multiple ZooKeeper clusters somewhere, I’d say, for now, that’s over engineering.

That said, it is potentially breaking, so I’ll go ahead & yank the minor-level release I cut for this, and issue a breaking release.

jeffbyrnes added a commit that referenced this pull request Oct 23, 2017
This reverts commit 6af1f32, reversing
changes made to 6620179.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants