From fc2d1d6ca913df0c0a0a9d2f62f27aace755eace Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Wed, 21 Mar 2018 17:08:25 +0300 Subject: [PATCH 1/5] fix dashboard version cleanup on large datasets --- pkg/services/sqlstore/dashboard_version.go | 39 ++++++++----------- .../sqlstore/dashboard_version_test.go | 2 +- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index 547f62628f3..3644af77355 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -1,7 +1,7 @@ package sqlstore import ( - "strings" + "fmt" "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" @@ -69,36 +69,31 @@ func GetDashboardVersions(query *m.GetDashboardVersionsQuery) error { func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { return inTransaction(func(sess *DBSession) error { - versions := []DashboardVersionExp{} versionsToKeep := setting.DashboardVersionsToKeep - if versionsToKeep < 1 { versionsToKeep = 1 } - err := sess.Table("dashboard_version"). - Select("dashboard_version.id, dashboard_version.version, dashboard_version.dashboard_id"). - Where(`dashboard_id IN ( - SELECT dashboard_id FROM dashboard_version - GROUP BY dashboard_id HAVING COUNT(dashboard_version.id) > ? - )`, versionsToKeep). - Desc("dashboard_version.dashboard_id", "dashboard_version.version"). - Find(&versions) + // 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. + versionIdsToDeleteSybqueryTemplate := `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 - %v` + versionIdsToDeleteSubquery := fmt.Sprintf(versionIdsToDeleteSybqueryTemplate, versionsToKeep) + deleteExpiredSql := fmt.Sprintf(`DELETE FROM dashboard_version WHERE id IN (%s)`, versionIdsToDeleteSubquery) + expiredResponse, err := sess.Exec(deleteExpiredSql) if err != nil { return err } - - // Keep last versionsToKeep versions and delete other - versionIdsToDelete := getVersionIDsToDelete(versions, versionsToKeep) - if len(versionIdsToDelete) > 0 { - deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` - expiredResponse, err := sess.Exec(deleteExpiredSql, versionIdsToDelete...) - if err != nil { - return err - } - cmd.DeletedRows, _ = expiredResponse.RowsAffected() - } + cmd.DeletedRows, _ = expiredResponse.RowsAffected() return nil }) diff --git a/pkg/services/sqlstore/dashboard_version_test.go b/pkg/services/sqlstore/dashboard_version_test.go index 1b74e7847c4..151dc7c4be2 100644 --- a/pkg/services/sqlstore/dashboard_version_test.go +++ b/pkg/services/sqlstore/dashboard_version_test.go @@ -136,7 +136,7 @@ func TestDeleteExpiredVersions(t *testing.T) { err := DeleteExpiredVersions(&m.DeleteExpiredVersionsCommand{}) So(err, ShouldBeNil) - query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1} + query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWrite} GetDashboardVersions(&query) So(len(query.Result), ShouldEqual, versionsToWrite) From f976b690ca4a208c827077eabf4d2d7e856ba076 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Wed, 21 Mar 2018 20:26:27 +0300 Subject: [PATCH 2/5] limit number of rows deleted by dashboard version cleanup --- pkg/services/sqlstore/dashboard_version.go | 53 ++++++++-------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index 3644af77355..754bcd504a7 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -2,6 +2,7 @@ package sqlstore import ( "fmt" + "strings" "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" @@ -69,6 +70,8 @@ func GetDashboardVersions(query *m.GetDashboardVersionsQuery) error { func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { return inTransaction(func(sess *DBSession) error { + const MAX_VERSIONS_TO_DELETE = 100 + versionsToKeep := setting.DashboardVersionsToKeep if versionsToKeep < 1 { versionsToKeep = 1 @@ -83,12 +86,25 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { 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 + ) AS vtd + WHERE dashboard_version.dashboard_id=vtd.dashboard_id AND version < vtd.min + vtd.count - %v` versionIdsToDeleteSubquery := fmt.Sprintf(versionIdsToDeleteSybqueryTemplate, versionsToKeep) - deleteExpiredSql := fmt.Sprintf(`DELETE FROM dashboard_version WHERE id IN (%s)`, versionIdsToDeleteSubquery) + versions := []string{} + err := sess.SQL(versionIdsToDeleteSubquery).Find(&versions) + if err != nil { + return err + } + + // Don't delete more than MAX_VERSIONS_TO_DELETE version per time + limit := MAX_VERSIONS_TO_DELETE + if len(versions) < MAX_VERSIONS_TO_DELETE { + limit = len(versions) + } + versions = versions[:limit] + + deleteExpiredSql := fmt.Sprintf(`DELETE FROM dashboard_version WHERE id IN (%s)`, strings.Join(versions, `,`)) expiredResponse, err := sess.Exec(deleteExpiredSql) if err != nil { return err @@ -98,34 +114,3 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { return nil }) } - -// Short version of DashboardVersion for getting expired versions -type DashboardVersionExp struct { - Id int64 `json:"id"` - DashboardId int64 `json:"dashboardId"` - Version int `json:"version"` -} - -func getVersionIDsToDelete(versions []DashboardVersionExp, versionsToKeep int) []interface{} { - versionIds := make([]interface{}, 0) - - if len(versions) == 0 { - return versionIds - } - - currentDashboard := versions[0].DashboardId - count := 0 - for _, v := range versions { - if v.DashboardId == currentDashboard { - count++ - } else { - count = 1 - currentDashboard = v.DashboardId - } - if count > versionsToKeep { - versionIds = append(versionIds, v.Id) - } - } - - return versionIds -} From 3f85fcce2ddc0ff81f83565373d218fdef2dfb3f Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Wed, 21 Mar 2018 21:01:15 +0300 Subject: [PATCH 3/5] refactor: dashboard version cleanup --- pkg/services/sqlstore/dashboard_version.go | 26 ++++++++++++---------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index 754bcd504a7..54c858df7de 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -81,7 +81,7 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { // 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. - versionIdsToDeleteSybqueryTemplate := `SELECT id + versionIdsToDeleteSubqueryTemplate := `SELECT id FROM dashboard_version, ( SELECT dashboard_id, count(version) as count, min(version) as min FROM dashboard_version @@ -90,26 +90,28 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { WHERE dashboard_version.dashboard_id=vtd.dashboard_id AND version < vtd.min + vtd.count - %v` - versionIdsToDeleteSubquery := fmt.Sprintf(versionIdsToDeleteSybqueryTemplate, versionsToKeep) - versions := []string{} - err := sess.SQL(versionIdsToDeleteSubquery).Find(&versions) + versionIdsToDeleteSubquery := fmt.Sprintf(versionIdsToDeleteSubqueryTemplate, versionsToKeep) + var versionIdsToDelete []interface{} + err := sess.SQL(versionIdsToDeleteSubquery).Find(&versionIdsToDelete) if err != nil { return err } // Don't delete more than MAX_VERSIONS_TO_DELETE version per time limit := MAX_VERSIONS_TO_DELETE - if len(versions) < MAX_VERSIONS_TO_DELETE { - limit = len(versions) + if len(versionIdsToDelete) < MAX_VERSIONS_TO_DELETE { + limit = len(versionIdsToDelete) } - versions = versions[:limit] + versionIdsToDelete = versionIdsToDelete[:limit] - deleteExpiredSql := fmt.Sprintf(`DELETE FROM dashboard_version WHERE id IN (%s)`, strings.Join(versions, `,`)) - expiredResponse, err := sess.Exec(deleteExpiredSql) - if err != nil { - return err + if len(versionIdsToDelete) > 0 { + deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` + expiredResponse, err := sess.Exec(deleteExpiredSql, versionIdsToDelete...) + if err != nil { + return err + } + cmd.DeletedRows, _ = expiredResponse.RowsAffected() } - cmd.DeletedRows, _ = expiredResponse.RowsAffected() return nil }) From 2ade0881b164437fb68870fdd1ae43fa66612923 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Wed, 21 Mar 2018 22:48:17 +0300 Subject: [PATCH 4/5] minor refactor of dashboard version cleanup --- pkg/services/sqlstore/dashboard_version.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index 54c858df7de..d91b5727545 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -1,7 +1,6 @@ package sqlstore import ( - "fmt" "strings" "github.com/grafana/grafana/pkg/bus" @@ -81,18 +80,17 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { // 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. - versionIdsToDeleteSubqueryTemplate := `SELECT id + versionIdsToDeleteSubquery := `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 - %v` + AND version < vtd.min + vtd.count - ?` - versionIdsToDeleteSubquery := fmt.Sprintf(versionIdsToDeleteSubqueryTemplate, versionsToKeep) var versionIdsToDelete []interface{} - err := sess.SQL(versionIdsToDeleteSubquery).Find(&versionIdsToDelete) + err := sess.SQL(versionIdsToDeleteSubquery, versionsToKeep).Find(&versionIdsToDelete) if err != nil { return err } From 0ffcea08c7188c6760322160b0bcfea1374a086f Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Thu, 22 Mar 2018 15:18:05 +0300 Subject: [PATCH 5/5] dashboard version cleanup: more tests and refactor --- pkg/services/sqlstore/dashboard_version.go | 20 +++++++++---------- .../sqlstore/dashboard_version_test.go | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/pkg/services/sqlstore/dashboard_version.go b/pkg/services/sqlstore/dashboard_version.go index d91b5727545..1f2850b2021 100644 --- a/pkg/services/sqlstore/dashboard_version.go +++ b/pkg/services/sqlstore/dashboard_version.go @@ -67,10 +67,10 @@ func GetDashboardVersions(query *m.GetDashboardVersionsQuery) error { return nil } +const MAX_VERSIONS_TO_DELETE = 100 + func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { return inTransaction(func(sess *DBSession) error { - const MAX_VERSIONS_TO_DELETE = 100 - versionsToKeep := setting.DashboardVersionsToKeep if versionsToKeep < 1 { versionsToKeep = 1 @@ -80,27 +80,25 @@ func DeleteExpiredVersions(cmd *m.DeleteExpiredVersionsCommand) error { // 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. - versionIdsToDeleteSubquery := `SELECT id + 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 - ?` + ) AS vtd + WHERE dashboard_version.dashboard_id=vtd.dashboard_id + AND version < vtd.min + vtd.count - ?` var versionIdsToDelete []interface{} - err := sess.SQL(versionIdsToDeleteSubquery, versionsToKeep).Find(&versionIdsToDelete) + err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep).Find(&versionIdsToDelete) if err != nil { return err } // Don't delete more than MAX_VERSIONS_TO_DELETE version per time - limit := MAX_VERSIONS_TO_DELETE - if len(versionIdsToDelete) < MAX_VERSIONS_TO_DELETE { - limit = len(versionIdsToDelete) + if len(versionIdsToDelete) > MAX_VERSIONS_TO_DELETE { + versionIdsToDelete = versionIdsToDelete[:MAX_VERSIONS_TO_DELETE] } - versionIdsToDelete = versionIdsToDelete[:limit] if len(versionIdsToDelete) > 0 { deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)` diff --git a/pkg/services/sqlstore/dashboard_version_test.go b/pkg/services/sqlstore/dashboard_version_test.go index 151dc7c4be2..a6403755d05 100644 --- a/pkg/services/sqlstore/dashboard_version_test.go +++ b/pkg/services/sqlstore/dashboard_version_test.go @@ -141,5 +141,25 @@ 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 + for i := 0; i < versionsToWriteBigNumber-versionsToWrite; i++ { + updateTestDashboard(savedDash, map[string]interface{}{ + "tags": "different-tag", + }) + } + + err := DeleteExpiredVersions(&m.DeleteExpiredVersionsCommand{}) + So(err, ShouldBeNil) + + query := m.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWriteBigNumber} + GetDashboardVersions(&query) + + // 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) + }) }) }