From 2ecc7f22b9775aa4939b1fc7d1fe1aa8d3271032 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 6 Apr 2020 11:44:14 -0400 Subject: [PATCH] delete old dashboard versions in multiple batches (#23348) --- pkg/services/sqlstore/dashboard_version.go | 81 ++++++++++++------- .../sqlstore/dashboard_version_test.go | 15 ++-- 2 files changed, 59 insertions(+), 37 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index 602d58d5715..30e02be0c07 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -67,49 +67,68 @@ func GetDashboardVersions(query *models.GetDashboardVersionsQuery) error { return nil } -const MAX_VERSIONS_TO_DELETE = 100 +const MAX_VERSIONS_TO_DELETE_PER_BATCH = 100 +const MAX_VERSION_DELETION_BATCHES = 50 func DeleteExpiredVersions(cmd *models.DeleteExpiredVersionsCommand) error { - return inTransaction(func(sess *DBSession) error { - versionsToKeep := setting.DashboardVersionsToKeep - if versionsToKeep < 1 { - versionsToKeep = 1 - } + return deleteExpiredVersions(cmd, MAX_VERSIONS_TO_DELETE_PER_BATCH, MAX_VERSION_DELETION_BATCHES) +} - // Idea of this query is finding version IDs to delete based on formula: - // min_version_to_keep = min_version + (versions_count - versions_to_keep) - // where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep - // versions, but in some cases (when versions are sparse) this number may be more. - versionIdsToDeleteQuery := `SELECT id - FROM dashboard_version, ( - SELECT dashboard_id, count(version) as count, min(version) as min - FROM dashboard_version - GROUP BY dashboard_id - ) AS vtd - WHERE dashboard_version.dashboard_id=vtd.dashboard_id - AND version < vtd.min + vtd.count - ?` +func deleteExpiredVersions(cmd *models.DeleteExpiredVersionsCommand, perBatch int, maxBatches int) error { + versionsToKeep := setting.DashboardVersionsToKeep + if versionsToKeep < 1 { + versionsToKeep = 1 + } - var versionIdsToDelete []interface{} - err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep).Find(&versionIdsToDelete) - if err != nil { - return err - } + for batch := 0; batch < maxBatches; batch++ { + deleted := int64(0) - // Don't delete more than MAX_VERSIONS_TO_DELETE version per time - if len(versionIdsToDelete) > MAX_VERSIONS_TO_DELETE { - versionIdsToDelete = versionIdsToDelete[:MAX_VERSIONS_TO_DELETE] - } + batchErr := inTransaction(func(sess *DBSession) error { + // Idea of this query is finding version IDs to delete based on formula: + // min_version_to_keep = min_version + (versions_count - versions_to_keep) + // where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep + // versions, but in some cases (when versions are sparse) this number may be more. + versionIdsToDeleteQuery := `SELECT id + FROM dashboard_version, ( + SELECT dashboard_id, count(version) as count, min(version) as min + FROM dashboard_version + GROUP BY dashboard_id + ) AS vtd + WHERE dashboard_version.dashboard_id=vtd.dashboard_id + AND version < vtd.min + vtd.count - ? + LIMIT ?` + + var versionIdsToDelete []interface{} + err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep, perBatch).Find(&versionIdsToDelete) + if err != nil { + return err + } + + if len(versionIdsToDelete) < 1 { + return nil + } - if len(versionIdsToDelete) > 0 { deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` sqlOrArgs := append([]interface{}{deleteExpiredSql}, versionIdsToDelete...) expiredResponse, err := sess.Exec(sqlOrArgs...) if err != nil { return err } - cmd.DeletedRows, _ = expiredResponse.RowsAffected() + + deleted, err = expiredResponse.RowsAffected() + return err + }) + + if batchErr != nil { + return batchErr } - return nil - }) + cmd.DeletedRows += deleted + + if deleted < int64(perBatch) { + break + } + } + + return nil } diff --git a/pkg/services/sqlstore/dashboard_version_test.go b/pkg/services/sqlstore/dashboard_version_test.go index a00b022e607..5b15db5ab07 100644 --- a/pkg/services/sqlstore/dashboard_version_test.go +++ b/pkg/services/sqlstore/dashboard_version_test.go @@ -131,7 +131,7 @@ func TestDeleteExpiredVersions(t *testing.T) { So(query.Result[0].Version, ShouldEqual, versionsToWrite) }) - Convey("Don't delete anything if there're no expired versions", func() { + Convey("Don't delete anything if there are no expired versions", func() { setting.DashboardVersionsToKeep = versionsToWrite err := DeleteExpiredVersions(&models.DeleteExpiredVersionsCommand{}) @@ -144,15 +144,18 @@ func TestDeleteExpiredVersions(t *testing.T) { So(len(query.Result), ShouldEqual, versionsToWrite) }) - Convey("Don't delete more than MAX_VERSIONS_TO_DELETE per iteration", func() { - versionsToWriteBigNumber := MAX_VERSIONS_TO_DELETE + versionsToWrite + Convey("Don't delete more than MAX_VERSIONS_TO_DELETE_PER_BATCH * MAX_VERSION_DELETION_BATCHES per iteration", func() { + perBatch := 10 + maxBatches := 10 + + versionsToWriteBigNumber := perBatch*maxBatches + versionsToWrite for i := 0; i < versionsToWriteBigNumber-versionsToWrite; i++ { updateTestDashboard(savedDash, map[string]interface{}{ "tags": "different-tag", }) } - err := DeleteExpiredVersions(&models.DeleteExpiredVersionsCommand{}) + err := deleteExpiredVersions(&models.DeleteExpiredVersionsCommand{}, perBatch, maxBatches) So(err, ShouldBeNil) query := models.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWriteBigNumber} @@ -161,8 +164,8 @@ func TestDeleteExpiredVersions(t *testing.T) { // Ensure we have at least versionsToKeep versions So(len(query.Result), ShouldBeGreaterThanOrEqualTo, versionsToKeep) - // Ensure we haven't deleted more than MAX_VERSIONS_TO_DELETE rows - So(versionsToWriteBigNumber-len(query.Result), ShouldBeLessThanOrEqualTo, MAX_VERSIONS_TO_DELETE) + // Ensure we haven't deleted more than perBatch * maxBatches rows + So(versionsToWriteBigNumber-len(query.Result), ShouldBeLessThanOrEqualTo, perBatch*maxBatches) }) }) }