evertrue / zookeeper-cookbook

Chef cookbook for installing and managing Zookeeper.
https://supermarket.chef.io/cookbooks/zookeeper
Apache License 2.0
81 stars 119 forks source link

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

Closed GolubevV closed 6 years ago

GolubevV commented 6 years ago

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.

GolubevV commented 6 years ago

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 commented 6 years ago

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.