Hi all,
I'd like to do a major refactor of our class
*brooklyn.location.jclouds.JcloudsLocation*. However, I suggest we try
to get a 0.7.0 release soon, and then do this refactoring.
_*Current issues*_
The class has grown very big (2642 lines, including comments and white
space).
The class has many responsibilities, which include:
* VM creation
* jclouds Template / TemplateOptions creation, for tailoring what VM
to create
* Retries and concurrency control, for VM creation
* Post-processing of VM (e.g. extracting most appropriate ip/port)
* VM customisation - for user creation
* VM customisation - other, e.g. generating/setting hostname, open
iptables, populating ~/.ssh/authorizedKeys, etc.
* JcloudsMachineLocation construction (to represent the created VM)
The class is hard to customise. There is a
brooklyn.location.jclouds.JcloudsLocationCustomizer, which provides
hooks for customising the provisioning process - it has six callbacks,
i.e. six interception points where the behaviour can be customised.
These cover many use-cases, but there are other use-cases that require
intercepting at other points.
Sub-classing is poorly documented and potentially brittle. For example,
in advanced-networking [1], it sub-classes JcloudsLocation to intercept
behaviour at appropriate points to set up private networks /
port-forwarding. We don't want to permanently support every protected
method as part of a "public" API. We want a more thought-out approach to
intercepting / augmenting behaviour. We want a clear statement about the
backwards compatibility guarantees we'll give for sub-classing.
There is quite complicated if-else behaviour. That would be very hard to
remove entirely (e.g. user creation + credentials is controlled by
several config keys, which thus requires if-else). However it could
likely be simplified. For example, windows versus linux differences
could be extracted into separate classes - the decision is made once,
and then the behaviour is different in each class.
_*Proposal*_
We should separate out responsibilities into separate classes. We should
use patterns, such as the "strategy", "command" and "factory" patterns,
to inject behaviour.
The injection should be customisable, so that power-users can customise
the behaviour (e.g. by replacing a given strategy class).
Of the "responsibilities" listed above, some of those fit nicely into
the refactoring (e.g. extract the "user creation" code into its own class).
Others map nicely to a cleaner separation of methods (e.g. concurrency
control not embedded in the middle of the method).
Others fit into a general "post-provisioning customisation" phase, which
could be its own class.
_*Next steps*_
Let's get 0.7.0 GA out of the door first.
I'm concious that the proposal above is vague, and a cynic could play
buzz-word bingo with pattern names.
However, if there's general agreement that this refactoring is worth
while, then let's do some incremental improvements (e.g. starting with
extracting into separate classes), and do a detailed review in the pull
requests.
Aled
[1] https://github.com/brooklyncentral/advanced-networking
