Add continuous export #949

arielshaqed · 2020-11-22T15:14:40Z

No description provided.

codecov-io · 2020-11-23T09:08:52Z

Codecov Report

Merging #949 (c52eefe) into master (24974e3) will decrease coverage by 0.22%.
The diff coverage is 24.17%.

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
- Coverage   44.46%   44.23%   -0.23%     
==========================================
  Files         143      143              
  Lines       11486    11560      +74     
==========================================
+ Hits         5107     5114       +7     
- Misses       5743     5807      +64     
- Partials      636      639       +3

Impacted Files	Coverage Δ
catalog/cataloger.go	`71.22% <ø> (ø)`
db/database.go	`0.62% <0.00%> (ø)`
export/export.go	`0.00% <0.00%> (ø)`
export/export_handler.go	`22.52% <6.66%> (-3.12%)`	⬇️
catalog/cataloger_export.go	`68.57% <94.73%> (+2.67%)`	⬆️
catalog/cataloger_commit.go	`82.60% <100.00%> (ø)`
catalog/cataloger_merge.go	`73.18% <100.00%> (ø)`
catalog/cataloger_create_branch.go	`84.90% <0.00%> (-3.78%)`	⬇️
catalog/cataloger_create_repository.go	`59.25% <0.00%> (-3.71%)`	⬇️
config/config.go	`33.33% <0.00%> (-0.46%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24974e3...c52eefe. Read the comment docs.

guy-har

Looks Good

guy-har · 2020-11-23T08:57:10Z

export/export.go

+	exportConfiguration, err := c.GetExportConfigurationForBranch(repo, branch)
+	if errors.Is(err, pgx.ErrNoRows) {


I think it would be better to return and check a cataloger error instead.

Yup! It actually returns a db error, the old code was incorrect. Thanks, good catch!

guy-har · 2020-11-23T09:07:25Z

export/export_handler.go

+	isContinuous, err := hasContinuousExport(h.cataloger, repo, branch)
+	if err != nil {
+		// FAIL this merge: if we were meant to export it and did not then in practice
+		// there was no merge.
+		return fmt.Errorf("check continuous export for merge %+v: %w", *merge, err)
+	}
+	if !isContinuous {
+		return nil
+	}
+	_, err = ExportBranchStart(h.parade, h.cataloger, repo, branch)
+	if errors.Is(err, ErrExportInProgress) {
+		err = nil
+	}


It might be nice to take it out to a function, given that exportCommitHook does the same

Thanks, that's a good idea! I thought it would be too hard because of logging and error messages, but it was actually easy...

catalog/cataloger.go

arielshaqed

Thanks!

Finishing CI, rebasing (might be painful) and merging...

catalog/cataloger.go

arielshaqed · 2020-11-23T09:33:29Z

export/export.go

+	exportConfiguration, err := c.GetExportConfigurationForBranch(repo, branch)
+	if errors.Is(err, pgx.ErrNoRows) {


Yup! It actually returns a db error, the old code was incorrect. Thanks, good catch!

arielshaqed · 2020-11-23T10:03:03Z

export/export_handler.go

+	isContinuous, err := hasContinuousExport(h.cataloger, repo, branch)
+	if err != nil {
+		// FAIL this merge: if we were meant to export it and did not then in practice
+		// there was no merge.
+		return fmt.Errorf("check continuous export for merge %+v: %w", *merge, err)
+	}
+	if !isContinuous {
+		return nil
+	}
+	_, err = ExportBranchStart(h.parade, h.cataloger, repo, branch)
+	if errors.Is(err, ErrExportInProgress) {
+		err = nil
+	}


Thanks, that's a good idea! I thought it would be too hard because of logging and error messages, but it was actually easy...

Handles a commit or merge occurring concurrently with an export.

Hook onto commits and merges to start an export. This export will attempt to export to the branch tip.

This is actually in [the docs](https://golang.org/pkg/time/#pkg-constants): To convert an integer number of units to a Duration, multiply: ``` seconds := 10 fmt.Print(time.Duration(seconds)*time.Second) // prints 10s ``` That's due to shortcomings of the Go type system. But this way is safer, it avoids conversion to an int type.

Most notably, avoid double updates in ExportBranchDone.

Yup, it ends up checking for the line length. 132 columns are back!!?! (http://www.righto.com/2019/01/accounting-machines-ibm-1403-and-why.html)

Also extract a smaller interface `cataloger.Exporter`

Package `scany` depends on a different version of `pgx` than the rest of lakeFS. So `errors.Is(err, pgx.ErrNoRows)` fails. Luckily it (sort-of) knows of this issue and wraps this call inside it as `pgxscan.NotFound`. Also make `ErrNotFound` wrap `pgx.ErrNoRows` rather than a new error.

Catalog methods do not support transactions anyway. So there is no way for a hook to use its transaction. OTOH, running inside the transaction means all catalog methods that the hook calls will NOT see the commit or merge. So fix that.

arielshaqed · 2020-11-25T08:38:42Z

@nopcoder please verify the rebase went well, I made a few changes (but fewer than I expected...).

catalog/cataloger.go

catalog/mvcc/cataloger_commit_test.go

catalog/mvcc/cataloger_diff_test.go

catalog/mvcc/cataloger_merge_test.go

export/export.go

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

arielshaqed

Thanks!

catalog/cataloger.go

catalog/mvcc/cataloger_diff_test.go

export/export.go

…into feature/534-continuous

arielshaqed · 2020-11-26T06:44:05Z

Yay! 🥳

Pulling, thanks @guy-har + @nopcoder ...

arielshaqed requested a review from guy-har November 22, 2020 15:14

arielshaqed force-pushed the feature/534-continuous branch from 529ddbf to c52eefe Compare November 23, 2020 08:58

guy-har approved these changes Nov 23, 2020

View reviewed changes

arielshaqed commented Nov 23, 2020

View reviewed changes

arielshaqed force-pushed the feature/534-continuous branch 4 times, most recently from 737b336 to b1621a9 Compare November 25, 2020 07:34

arielshaqed added 14 commits November 25, 2020 09:36

Continuous export: try to start a new export after each export

cfae470

Handles a commit or merge occurring concurrently with an export.

Continuous export: start a new export after each commit or merge

a4b3ec0

Hook onto commits and merges to start an export. This export will attempt to export to the branch tip.

Test (empty) diff between a ref and itself

f32712f

Improve logging and clean up flows

b3acce7

Most notably, avoid double updates in ExportBranchDone.

[checks] use a shorter error line to pass err113 golangci check

4871e49

Yup, it ends up checking for the line length. 132 columns are back!!?! (http://www.righto.com/2019/01/accounting-machines-ibm-1403-and-why.html)

[CR] Refactor merge & commit hooks

40fec4b

Also extract a smaller interface `cataloger.Exporter`

[post-rebase] Revert "status" back to "state" in field names

d55eb5d

WIP try to fix db notfound errs

5743209

[CR] GetPrimitive doesn't call pgxscan, use pgx directly there

3e63322

[WIP] avoid empty exports

43e6261

Run hooks outside of transactions

5e77681

Catalog methods do not support transactions anyway. So there is no way for a hook to use its transaction. OTOH, running inside the transaction means all catalog methods that the hook calls will NOT see the commit or merge. So fix that.

Post-rebase fixes

74527c9

arielshaqed force-pushed the feature/534-continuous branch from b1621a9 to 74527c9 Compare November 25, 2020 08:30

arielshaqed requested a review from nopcoder November 25, 2020 08:38

nopcoder suggested changes Nov 25, 2020

View reviewed changes

arielshaqed and others added 2 commits November 25, 2020 13:26

[CR] Flatten if/elses in catalog/mvcc/cataloger_merge_test.go

b35b36e

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

[CR] pass hook values by value

99770f8

arielshaqed commented Nov 25, 2020

View reviewed changes

catalog/cataloger.go Outdated Show resolved Hide resolved

catalog/mvcc/cataloger_diff_test.go Show resolved Hide resolved

export/export.go Show resolved Hide resolved

arielshaqed requested a review from nopcoder November 25, 2020 11:58

Merge branch 'feature/534-continuous' of github.com:treeverse/lakeFS …

eeeed6d

…into feature/534-continuous

arielshaqed force-pushed the feature/534-continuous branch from 4a3a5ce to eeeed6d Compare November 25, 2020 15:04

nopcoder approved these changes Nov 25, 2020

View reviewed changes

arielshaqed merged commit 84a52ee into master Nov 26, 2020

arielshaqed deleted the feature/534-continuous branch November 26, 2020 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add continuous export #949

Add continuous export #949

arielshaqed commented Nov 22, 2020

codecov-io commented Nov 23, 2020

guy-har left a comment

guy-har Nov 23, 2020

arielshaqed Nov 23, 2020

guy-har Nov 23, 2020

arielshaqed Nov 23, 2020

arielshaqed left a comment

arielshaqed Nov 23, 2020

arielshaqed Nov 23, 2020

arielshaqed commented Nov 25, 2020

arielshaqed left a comment

arielshaqed commented Nov 26, 2020

		exportConfiguration, err := c.GetExportConfigurationForBranch(repo, branch)
		if errors.Is(err, pgx.ErrNoRows) {

Add continuous export #949

Add continuous export #949

Conversation

arielshaqed commented Nov 22, 2020

codecov-io commented Nov 23, 2020

Codecov Report

guy-har left a comment

Choose a reason for hiding this comment

guy-har Nov 23, 2020

Choose a reason for hiding this comment

arielshaqed Nov 23, 2020

Choose a reason for hiding this comment

guy-har Nov 23, 2020

Choose a reason for hiding this comment

arielshaqed Nov 23, 2020

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed Nov 23, 2020

Choose a reason for hiding this comment

arielshaqed Nov 23, 2020

Choose a reason for hiding this comment

arielshaqed commented Nov 25, 2020

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed commented Nov 26, 2020

Yay! 🥳