Github user nathanejohnson commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1635#discussion_r76547668
--- Diff:
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse,
@BeanParam DeploymentPlan plan, E
boolean stop(String caller) throws ResourceUnavailableException,
CloudException;
/**
+ * Stop the virtual machine, optionally force
+ *
+ */
+ boolean stop(String caller, boolean forced) throws
ResourceUnavailableException, CloudException;
--- End diff --
For better or worse, I was just trying to follow the lead of the actual
method that does the work in question, namely advanceStop, and some of the
methods called from advanceStop. I have no issue changing the name to have
force in it and skip the bool flag (lest Fowler think I'm a bad programmer),
but I am not thinking that I'd like to change the signature of advanceStop or
any other the other methods that eventually get called in the chain where there
is a bool flag in it. I think the much bigger issue with this bit of code is
that the refactor that lead to this issue in the first place abstracted the
call to advanceStop under so many layers that it made it really hard to notice
that functionality was reduced in the first place, to the point where the
author nor anyone reviewing caught it. If I was to place serious energy into
refactoring, I'd probably start with undoing a lot of that abstraction, but
that would require me being familiar with a lot more of the codebase than I
currently am. Also, more immediately, I want to come up with a good test for
this, but so far haven't been able to come up with something reliable /
reproducible under kvm.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
