Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Remove duplicated operations from remove application transaction. #7219
Conversation
jameinel
requested changes
Apr 10, 2017
most of this doesn't seem like redundant data being requested, can you clarify?
| - }, { | ||
| - C: settingsC, | ||
| - Id: a.settingsKey(), | ||
| - Remove: true, |
jameinel
Apr 10, 2017
Owner
how is this redundant?
One is removing the values from applications collection, the other is removing the application-specific settings from the settings collection.
I'm pretty sure we do need both.
anastasiamac
Apr 10, 2017
Member
This operation gets added, tracing through to appCharmDecRefOps(...), at https://github.com/juju/juju/blob/develop/state/charmref.go#L80.
If we are to keep the remove settings operation I suggest we get rid of, we end up wtih 2 remove settings operations that are exactly the same. I have added my observations into the pastebin :)
jameinel
Apr 10, 2017
Owner
from the pastebin I see 3 calls to remove the settings key. This seems to be removing one of them. Do you know where the other is?
Are we sure this is the only path that triggers this code?
anastasiamac
Apr 10, 2017
Member
Yes, I do :)
- here (propose to remove in this PR)
- in appCharmDecRefOps
- in finalAppCharmRemoveOps (propose to remove in this PR)
There will still be potential for duplication since we will still call finalAppCharmRemoveOps(...) inside appCharmDecRefOps(...) but it will be a lot less frequent then now.
| @@ -290,7 +286,6 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| ops = append(ops, charmOps...) | ||
| - ops = append(ops, finalAppCharmRemoveOps(name, curl)...) |
jameinel
Apr 10, 2017
Owner
I don't know what 'finalAppCharmRemoveOps' are off hand. Is it the sort of thing that destroys a machine one the last unit on that machine is removed? That doesn't sound like something we want to drop, though I would think the tests should be testing that.
anastasiamac
Apr 10, 2017
Member
finalAppCharmRemoveOps(...) is already called inside appCharmDecRefOps(...) whcih this method calls.
In other words, we add operations from finalAppCharmRemoveOps then we call appCharmDecRefOps which calls finalAppCharmRemoveOps and we add the same operations again :)
We end up with 2 sets of the same operations. Also the correct call is, in fact, appCharmDecRefOps(...) it contains the logic that detects if the unit to be removed is the final unit and adds "final" operations. Keeping this "naked" [1] call to finalAppCharmRemoveOps(...) here, means that we always end up with these operations whether they are needed or not.
[1] not protected by logic - am I final?
| - err := app.Destroy() |
jameinel
approved these changes
Apr 10, 2017
I haven't audited the rest of the code to know where these are being used, and where not, but removing redundant duplications is good.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
anastasiamac commentedApr 10, 2017
Description of change
Removed redundant duplicated operations from remove application/unit transaction.
Call to appCharmDecRefOps takes care of both application settings removal operations as well as removal of charm related operations that should take affect when the final application unit is removed. In fact, the latter is properly conditionally added.
QA steps
Internal change - if QA observes the internal transaction collection and the operations that it contains, then the transaction that removes application will no longer contain duplicate operations.
This is how it looked prior to the change, grouped by duplication not the order in transaction: http://pastebin.ubuntu.com/24332772/
Documentation changes
N/A, internal change.
Bug reference
N/A, found during investigation related to the destruction of the model with an empty application (no machines or units)