Find expired and active commits #2069

johnnyaug · 2021-06-06T12:04:37Z

Closes #2067

API endpoint to set/get GC rules for a repository. Rules are saved in S3 under under _lakefs/retention/rules.
API endpoint to prepare a set of active/expired commits according to the GC rules. The result is saved as a CSV file under _lakefs/retention/commits.

itaiad200 · 2021-06-16T08:03:02Z

api/swagger.yml

+    GarbageCollectionPrepareRequest:
+      type: object
+      properties:
+        previous_result_path:


Could you add an example? Is it an object-store path or a lakeFS one?

Changed this to run-id and added example

itaiad200 · 2021-06-16T08:05:01Z

api/swagger.yml

+    GarbageCollectionRules:
+      type: object
+      properties:
+        default_retention_days:


What's the relation between a rule's retention_days and the global default_retention_days?

itaiad200 · 2021-06-16T08:05:28Z

api/swagger.yml

+      tags:
+        - retention
+      operationId: getGarbageCollectionRules
+      responses:


Missing 404 for missing repo.

itaiad200 · 2021-06-16T08:06:23Z

api/swagger.yml

+          application/json:
+            schema:
+              $ref: "#/components/schemas/GarbageCollectionRules"
+      responses:


Missing 404.

itaiad200 · 2021-06-16T08:11:27Z

pkg/graveler/retention/garbage_collection.go

+}
+
+func NewRuleManager(blockAdapter block.Adapter, blockStoragePrefix string) *RuleManager {
+	return &RuleManager{blockAdapter: blockAdapter, configurationFileSuffix: fmt.Sprintf("/%s/retention/rules/config.json", blockStoragePrefix)}


Set retention/rules/config.json as const.

itaiad200 · 2021-06-16T09:22:31Z

pkg/graveler/ref/expired_commits_test.go

@@ -0,0 +1,214 @@
+package ref


Great tests!

itaiad200 · 2021-06-16T09:28:01Z

pkg/catalog/catalog.go

+		for _, commitRow := range previousCommits {
+			previouslyExpiredCommits = append(previouslyExpiredCommits, graveler.CommitID(commitRow[1]))
+		}


They are not all expired, are they?

good catch, fixing

itaiad200 · 2021-06-16T09:28:42Z

pkg/catalog/catalog.go

+	return c.Store.SetRetentionRules(ctx, graveler.RepositoryID(repositoryID), rules)
+}
+
+func (c *Catalog) PrepareExpiredCommits(ctx context.Context, repository string, previousResultPath string) (string, error) {


Maybe extract the csv parsing to a designated parser?
It should also handle the schema of this file which is currently static.

itaiad200 · 2021-06-16T09:29:33Z

pkg/catalog/catalog.go

+			return "", err
+		}
+	}
+	csvWriter.Flush()


Should you also close it?

Nothing to close here: we are writing to a strings.Builder

itaiad200 · 2021-06-16T09:30:46Z

pkg/catalog/catalog.go

+	csvWriter.Flush()
+	commitsStr := b.String()
+	runID := uuid.New().String()
+	path := fmt.Sprintf("_lakefs/retention/commits/run_id=%s/commits.csv", runID)


_lakefs prefix should be passed to you from the config.

Maybe add the /retention/commits/ part too.

Done the first one

guy-har

Looks Great!!
The code look great, liked the tests.

I have a few concerns about the API paths, the GarbageCollectionPrepareRequest contains the full path, I think it should only contain the runID. the prepare path could be a bit confusing. There are places the name garbage collection is used and other where the name retention is used, which is OK, but got me thinking that, when working on lifecycle we might decide to change the API (or align it somehow), at that point it will be a breaking change. I suggest we leave the API for setting and getting configurations for later, what do you think?

guy-har · 2021-06-16T07:53:21Z

api/swagger.yml

+        previous_result_path:
+          type: string
+          description: path to the result of a previous successful GC job


Isn't run ID enough?

guy-har · 2021-06-16T07:53:36Z

api/swagger.yml

+      properties:
+        path:
+          type: string
+          description: path to a dataset of commits
+


guy-har · 2021-06-16T07:59:52Z

api/swagger.yml


+
+  /repositories/{repository}/gc/prepare:


change path to be more specific
prepare could also mean prepare the expired addresses data

guy-har · 2021-06-16T08:30:55Z

pkg/graveler/graveler.go

+
+	GetRetentionRules(ctx context.Context, repositoryID RepositoryID) (*RetentionRules, error)
+
+	SetRetentionRules(ctx context.Context, repositoryID RepositoryID, rules *RetentionRules) error
+


Is this retention rules or garbage collection rules?
In the future we will also have lifecycle rules.
Not sure if it will be together but it could get a bit confusing.

Maybe we should give it another thought before adding the API routes...

Rename to garbage collection rules

guy-har · 2021-06-16T08:38:56Z

pkg/graveler/graveler.go

@@ -856,6 +879,32 @@ func (g *Graveler) GetStagingToken(ctx context.Context, repositoryID RepositoryI
 	return &branch.StagingToken, nil
 }

+func (g *Graveler) GetRetentionRules(ctx context.Context, repositoryID RepositoryID) (*RetentionRules, error) {
+	// TODO use "_lakefs" from configuration


still relevant?

guy-har · 2021-06-16T08:39:42Z

pkg/graveler/graveler.go

+}
+
+func (g *Graveler) SetRetentionRules(ctx context.Context, repositoryID RepositoryID, rules *RetentionRules) error {
+	// TODO use "_lakefs" from configuration


still relevant?

guy-har · 2021-06-16T08:50:03Z

pkg/catalog/catalog.go

+	csvWriter.Flush()
+	commitsStr := b.String()
+	runID := uuid.New().String()
+	path := fmt.Sprintf("_lakefs/retention/commits/run_id=%s/commits.csv", runID)


Don't use hardcoded path, get _lakefs from the configurations. maybe even use a function to get the path

guy-har · 2021-06-16T08:54:23Z

pkg/api/controller.go

+func (c *Controller) PrepareGarbageCollectionCommits(w http.ResponseWriter, r *http.Request, body PrepareGarbageCollectionCommitsJSONRequestBody, repository string) {
+	if !c.authorize(w, r, []permissions.Permission{
+		{
+			Action:   permissions.ListObjectsAction,


Shouldn't be same as log commits ?
ReadBranchAction

Since I need this permission for all branches, and not just one, I'm adding a dedicated permission for this: retention:PrepareGarbageCollectionCommits

nopcoder

My concern is that prepare operation can take time to perform and can cause the operation to timeout - do we have numbers on how long it can take to process the request?

nopcoder · 2021-06-16T07:48:15Z

api/swagger.yml

+    GarbageCollectionCommits:
+      type: object
+      properties:
+        path:
+          type: string
+          description: path to a dataset of commits


can we add the type/format of the collection as a parameter or in the description?

api/swagger.yml

nopcoder · 2021-06-16T08:39:17Z

pkg/permissions/actions.go

+	GetGarbageCollectionRules = "retention:GetGarbageCollectionRules"
+	SetGarbageCollectionRules = "retention:SetGarbageCollectionRules"


Don't forget to add documentation https://docs.lakefs.io/reference/authorization.html - can be different PR as we can merge this one before the release

nopcoder · 2021-06-16T08:52:02Z

pkg/api/controller.go

+func (c *Controller) SetGarbageCollectionRules(w http.ResponseWriter, r *http.Request, body SetGarbageCollectionRulesJSONRequestBody, repository string) {
+	if !c.authorize(w, r, []permissions.Permission{
+		{
+			Action:   permissions.GetGarbageCollectionRules,


SetGarbageCollectionRules

nopcoder · 2021-06-16T13:50:00Z

pkg/catalog/catalog.go

+			return "", err
+		}
+		csvReader := csv.NewReader(previousRunReader)
+		previousCommits, err := csvReader.ReadAll()


prefer iteration as we don't keep the array of record and just append them into our own structure

Done (and moved to another place)

nopcoder · 2021-06-16T13:53:40Z

pkg/catalog/catalog.go

+			return "", err
+		}
+	}
+	csvWriter.Flush()


need to call Error() after this call to capture possible errors

nopcoder · 2021-06-16T13:57:03Z

pkg/catalog/catalog.go

+	b := &strings.Builder{}
+	csvWriter := csv.NewWriter(b)
+	for _, commitID := range expiredCommits {
+		err = csvWriter.Write([]string{string(commitID), strconv.FormatBool(true)})


use "true" or capture the value outside the loops

nopcoder · 2021-06-16T13:57:10Z

pkg/catalog/catalog.go

+		}
+	}
+	for _, commitID := range activeCommits {
+		err = csvWriter.Write([]string{string(commitID), strconv.FormatBool(false)})


use "false" or capture the value outside the loops

nopcoder · 2021-06-16T14:00:03Z

pkg/catalog/catalog.go

+	if err != nil {
+		return "", err
+	}
+	previouslyExpiredCommits := make([]graveler.CommitID, 0)


no need zero allocated slice for this one

Suggested change

previouslyExpiredCommits := make([]graveler.CommitID, 0)

var previouslyExpiredCommits []graveler.CommitID

Done (and moved to another place)

nopcoder

just remove the user a var for an empty struct - can just pass it

nopcoder · 2021-06-20T12:41:37Z

pkg/graveler/retention/expired_commits.go

+	"github.com/treeverse/lakefs/pkg/graveler"
+)
+
+var empty struct{}


no need for this one

johnnyaug added 23 commits June 1, 2021 13:45

find active commits

d466eb4

wip

0abc5c6

Merge branch 'master' into feature/find_active_commits

47f44f6

rename test

431c207

linter fixes

3763a54

fix test

3876eff

wip

2ddf880

Merge branch 'master' into feature/find_active_commits

40ee31f

wip

d37ec3a

Merge branch 'master' into feature/find_active_commits

de1eef4

wip

bb5823b

Merge branch 'master' into feature/find_active_commits

499667f

put everything in graveler

494239d

linting

8cb41dd

revert typo

196d946

finish api wiring

edff49c

fix sorting in test

0d1db93

gen files

8d1b96f

rules api

88edd93

gen

c8de6a5

Merge branch 'master' into feature/find_active_commits

6de0637

gen

759f5fe

use config

b18aca6

johnnyaug changed the title ~~Feature/find active commits~~ Find expired and active commits Jun 15, 2021

johnnyaug marked this pull request as ready for review June 15, 2021 14:41

johnnyaug requested review from nopcoder and guy-har June 15, 2021 14:41

itaiad200 requested changes Jun 16, 2021

View reviewed changes

guy-har requested changes Jun 16, 2021

View reviewed changes

Merge branch 'master' into feature/find_active_commits

63ff3b0

nopcoder suggested changes Jun 16, 2021

View reviewed changes

johnnyaug added 5 commits June 16, 2021 17:18

CR fixes: fix abstraction leaks, naming changes

3acef2f

CR fixes

d02f0c7

CR fixes

7ce29bc

gen clients

a7c306a

cr fixes

b5e6447

johnnyaug requested review from nopcoder, itaiad200 and guy-har June 20, 2021 09:33

Merge branch 'master' into feature/find_active_commits

111cf0c

johnnyaug mentioned this pull request Jun 20, 2021

docs: authorization policies update #2142

Merged

nopcoder suggested changes Jun 20, 2021

View reviewed changes

cr fix

ea99529

johnnyaug requested a review from nopcoder June 20, 2021 12:52

nopcoder approved these changes Jun 20, 2021

View reviewed changes

guy-har approved these changes Jun 21, 2021

View reviewed changes

johnnyaug merged commit af759c6 into master Jun 21, 2021

johnnyaug deleted the feature/find_active_commits branch June 21, 2021 07:38


		GetRetentionRules(ctx context.Context, repositoryID RepositoryID) (*RetentionRules, error)

		SetRetentionRules(ctx context.Context, repositoryID RepositoryID, rules *RetentionRules) error

		GetGarbageCollectionRules = "retention:GetGarbageCollectionRules"
		SetGarbageCollectionRules = "retention:SetGarbageCollectionRules"

	previouslyExpiredCommits := make([]graveler.CommitID, 0)
	var previouslyExpiredCommits []graveler.CommitID

Find expired and active commits #2069

Find expired and active commits #2069

Conversation

johnnyaug commented Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug commented Jun 6, 2021 •

edited

Loading