refactor(general): Index compaction enhancement #3901
Conversation
2bc321e to
5b2c2c3
Compare
5b2c2c3 to
b47ffba
Compare
| if totalSize >= totalSizeBytesThresholdHard { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
@Lyndon-Li Please remove this check from this PR (and the corresponding tests). At the moment, we are not going to change the conditions for compaction. Also, if it were to do it, then it should be separately in its own PR.
| if totalSize >= totalSizeBytesThresholdHard { | |
| return true | |
| } |
There was a problem hiding this comment.
If we don't add this change, I am afraid the epoch maintenance in the quick maintenance doesn't make any difference --- since the minEpochDuration is 24 hour, even though quick maintenance launches the epoch maintenance every 1 hour, it actually does nothing since the epoch is not advanced.
There was a problem hiding this comment.
Running epoch maintenance in quick maintenance would make a difference. Specifically regarding epoch advancement, it would advance it within an hour of the reaching the minimum epoch duration age. Otherwise, in the worst case scenario the epoch would be advance close to 24 hours after being reaching the minimum epoch age.
There was a problem hiding this comment.
Yes, but that is not enough, or the epoch advancing is not timely enough once a burst of index generation happens. The consequence is that the next index compaction takes huge memory.
There was a problem hiding this comment.
You can lower the minimum epoch duration for the repository.
There was a problem hiding this comment.
May I know the concern here? are we expecting the epochs to always be regular in terms of time?
There was a problem hiding this comment.
Is it safe?
Sorry, I didn't see this comment by the first time.
Below is my thinking:
- The index compaction won't delete the superseded indexes immediately. After they are compacted in a quick maintenance, they will be deleted in the following FULL maintenance ONLY when they are old enough (controlled by
CleanupSafetyMargin, by default 4 hours). - Therefore, adding the hard limit ostensibly increase the risk of data lose, but risk is actually decided by
CleanupSafetyMargin. If data safety is the concern, users should increaseCleanupSafetyMargin. - Additionally, at present, we support a customized duration time as shorter as 10min. So users should have already been using this experience --- advance the epoch in a short time.
Data safety is always with top priority, but scalability and performance is also important. I am not sure if the changes in this PR is 100% correct, but it brings a possible solution of considering the two aspects together.
So appreciate your further comments.
There was a problem hiding this comment.
You raise an important point about the safety of deleting compacted indexes. Indeed a safety margin is needed so clients that are in the process of reading an old index snapshot can read that snapshot.
The proposed change adds a new criterion for advancing the epoch (totalSize >= totalSizeBytesThresholdHard). The original protocol design uses 2 other criteria to reach consensus about:(a) when it is time to advance and epoch; and (b) when it safe to assume that the epoch will no longer be modified, so it can be compacted.
The new criterion appears to be violating one of the original assumptions. It may be that the proposed change is safe, but it is not yet clear, at least not to me.
There was a problem hiding this comment.
OK, I got you points and understand your concern of data safety.
And it is also a fact that the current behavior of epoch advancing brings burst burden to repo maintenance (and other operations which need to read the compacted indexes), as index creations are never linearly streamed along with time, but influx with backups.
Let's see if this brings problems to other cases, then we can come back to discuss this.
Changes: * test that quick maintenance runs when epoch manager is enabled * fix(general): run epoch maintenance for quick maintenance Change based on a known-to-be-safe portion of the changes proposed in #3901 * cleanup: pass epoch manager to `runTaskEpochMaintenanceQuick` The caller needs to get the epoch manager to determine whether or not the epoch manager is enabled. The caller now passes the epoch manager to `runTaskEpochMaintenanceQuick` * wrap the error inside runTaskEpochMaintenanceQuick
|
Mostly superseded by #4185. Closing for now. Credits to @Lyndon-Li for catching the issue and proposing the solution. Thanks again. |
Changes: * test that quick maintenance runs when epoch manager is enabled * fix(general): run epoch maintenance for quick maintenance Change based on a known-to-be-safe portion of the changes proposed in kopia#3901 * cleanup: pass epoch manager to `runTaskEpochMaintenanceQuick` The caller needs to get the epoch manager to determine whether or not the epoch manager is enabled. The caller now passes the epoch manager to `runTaskEpochMaintenanceQuick` * wrap the error inside runTaskEpochMaintenanceQuick
Changes: * test that quick maintenance runs when epoch manager is enabled * fix(general): run epoch maintenance for quick maintenance Change based on a known-to-be-safe portion of the changes proposed in kopia#3901 * cleanup: pass epoch manager to `runTaskEpochMaintenanceQuick` The caller needs to get the epoch manager to determine whether or not the epoch manager is enabled. The caller now passes the epoch manager to `runTaskEpochMaintenanceQuick` * wrap the error inside runTaskEpochMaintenanceQuick
runTaskEpochMaintenanceQuicknever gets called inrunQuickMaintenanceepochAdvanceOnTotalSizeBytesThresholdforshouldAdvance. Epoch is advanced when the total index size in the epoch exceeds the hard limit. In this way, we keep indexes in an epoch in a reasonable number/size, so that the index compaction for that epoch won't take extremely high CPU and memory