New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
state: delete status history in batches #8254
Conversation
When removing status history docs because a unit was destroyed, remove the docs in batches to avoid locking the collection for a long period of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How big are the batches by default?
lastUpdate = now | ||
} | ||
} | ||
} | ||
if err := iter.Close(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we are closing the iterator both here and in the calling function?
I guess this is fine as long as calling close twice is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close is documented as being idempotent.
@@ -208,16 +217,19 @@ func (p *collectionPruner) deleteInBatches(iter *mgo.Iter, logTemplate string, s | |||
|
|||
now := time.Now() | |||
if now.Sub(lastUpdate) >= historyPruneProgressSeconds*time.Second { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about the historyPruneProgressSeconds value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that if the deletion takes more than 15s in total, yes we do.
1000 documents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this a go then.
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
Backport status history deletion and mongo iterator closure to 2.2 ## Description of change Backport #8254 and #8255 to the 2.2 branch. ## QA steps Smoke test bootstrap/deploy/destroy-controller. ## Documentation changes None. ## Bug reference https://bugs.launchpad.net/juju/+bug/1733708 https://bugs.launchpad.net/juju/+bug/1739380
iter := history.Find(bson.D{{ | ||
globalKeyField, globalKey, | ||
}}).Select(bson.M{"_id": 1}).Iter() | ||
defer iter.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we could update this to at least log errors from Close()?
Description of change
When removing status history docs because
a unit was destroyed, remove the docs in
batches to avoid locking the collection
for a long period of time.
QA steps
smoke test bootstrap / deploy / remove-application
(check status history docs removed)
Also, I ran a before/after experiment:
After the change, takes ~10s less time overall (~80s -> ~70s)
Documentation changes
None.
Bug reference
May help with https://bugs.launchpad.net/juju/+bug/1733708
Also partially addresses https://bugs.launchpad.net/juju/+bug/1739380 (closes iterators in pruning code)