Generate tasks from diffs for exporting #803

arielshaqed · 2020-10-12T14:33:36Z

Part of #534.

Reviewers:

@guy-har for everything
@nopcoder for changes to diff format, and for Go, and I hope also everything.

arielshaqed · 2020-10-12T15:21:55Z

nopcoder · 2020-10-12T15:07:52Z

export/tasks_generator.go

+	"fmt"
+	"strings"
+
+	rinqueue "github.com/erikdubbelboer/ringqueue"


interesting - why rinqueue?

Auto-happened because of something Go-ish. There is a bug in ringqueue that the package is actually named rinqueue. Anything else causes fun with gofmt while saving etc.

nopcoder · 2020-10-12T15:08:43Z

export/tasks_generator.go

+
+const successFilename = "_lakefs_success"
+
+const CopyActipn = "export:copy"


s/Actipn/Action/

can scope them in one const - usually, it just telling me they are all part of the same gang

optional add a type

1+2 are great. Not doing 3, because Action is actually a string on TaskData.

nopcoder · 2020-10-12T19:34:19Z

export/tasks_generator.go

+	return *ret, true
+}
+
+func makeDirMatchCache(pred func(path string) bool) *dirMatchCache {


Suggested change

func makeDirMatchCache(pred func(path string) bool) *dirMatchCache {

func newDirMatchCache(pred func(path string) bool) *dirMatchCache {

any reason you prefer 'make' and not the usual 'new' prefix?

Value factories are called make in C++. Wait, you mean we're not using C++?!
(Done)

nopcoder · 2020-10-12T20:26:29Z

export/tasks_generator.go

+		dir = dirname(dir)
+		var ok bool
+		if ret, ok = dmc.upMatchCache[dir]; ok {
+			break
+		}
+		if dmc.pred(dir) {
+			copy := dir
+			ret = &copy
+			break
+		}
+		if dir == "" {
+			break
+		}
+	}
+	for dir = dirname(filename); dir != "" && (ret == nil || dir != *ret); dir = dirname(dir) {
+		dmc.upMatchCache[dir] = ret
+	}


does pred can return true for empty dir?
if empty dir got no meaning and we do not cache the result for empty dir, I think we can move the dir == "" check to be after dir = dirname(dir)

Good point: it certainly does make sense to return true for the empty directory.

Exporting the object and writing a test for it, so I can fix its behaviour (in the sense of not changing, and also in the sense of making it behave correctly).

nopcoder · 2020-10-12T20:27:24Z

export/tasks_generator.go

+	return &dirMatchCache{pred: pred, upMatchCache: make(map[string]*string)}
+}
+
+// generateTasksFromDiffs converts diffs into many tasks that depend on startTaskID, with a


s/generateTasksFromDiffs/GenerateTasksFromDiffs/

nopcoder · 2020-10-12T21:55:39Z

parade/ddl.go

+var ErrBadTypeConversion = errors.New("bad type")
+
+// nolint: stylecheck
+func (dst *TaskStatusCodeValue) Scan(src interface{}) error {


Usually, all the receivers have the same name (and optionally - one letter)

I know. But the sql package does it this way

more than one letter - but I don't think for the same type have a different name for the receiver (src, dst)

Sorry, it's pgtype that does this for Scan and Value. Will keep this code as it does follow the gold standard.

nopcoder · 2020-10-12T21:57:12Z

parade/ddl.go

+		return fmt.Errorf("cannot convert %T to TaskStatusCodeValue: %w", src, ErrBadTypeConversion)
+	}
+
+	if !(sc == TaskPending || sc == TaskInProgress || sc == TaskAborted || sc == TaskCompleted) {


can't commit on previous lines but check the code-doc of these of does not have the matched constant name

Not sure I understand what I need to do here.

Some of the tasks here got wrong code-doc comment - ex: TaskAborted is ABORTED

// TaskPending indicates a task is waiting for an actor to perform it (new or being // retried) TaskPending TaskStatusCodeValue = "pending" // IN_PROGRESS indicates a task is being performed by an actor. TaskInProgress TaskStatusCodeValue = "in-progress" // ABORTED indicates an actor has aborted this task with message, will not be reissued TaskAborted TaskStatusCodeValue = "aborted" // TaskCompleted indicates an actor has completed this task with message, will not reissued TaskCompleted TaskStatusCodeValue = "completed" // TaskInvalid is used by the API to report errors TaskInvalid TaskStatusCodeValue = "[invalid]"

Oh, thanks! I went from C++ style to Go style and missed the comments. It's part of parade, so on #812.

nopcoder · 2020-10-12T22:18:29Z

parade/parade_test.go

+	t.Helper()
+	rows, err := db.Query(`SELECT id FROM tasks WHERE id LIKE format('%s%%', $1::text)`, prefix)
+	if err != nil {
+		t.Errorf("[I] select remaining IDs for prefix %s: %s", prefix, err)


nopcoder · 2020-10-12T22:19:31Z

parade/parade_test.go

+	for rows.Next() {
+		var id parade.TaskID
+		if err = rows.Scan(&id); err != nil {
+			t.Errorf("[I] scan ID value: %s", err)


Fatalf - not sure you can continue to scan after that

Why not? It's just a broken row (e.g. parse failure). rows.Next is welcome to fail if the database is 🔥 on fire.

So you're actually right, the code is wrong, and I added a test that rows were successfully scanned.

You are right that calling Next after Scan that failed will probably exit. The code reader that sees Errorf will think that this error is something we can live with and continue - in this case adding the id to the slice, which is not what we wanted.

Not done, because of a difference in how we two think about test failures.

This is how Moketa the code monkey once tried to teach me how to test. Basically, stop the test only when continuing the test will fail on testing code. Otherwise continue and print more error messages to get more data about the failure. In this case I don't mind pushing a dud id onto the slice: it won't be the first error, but the remaining error messages might help me understand what went wrong. E.g. otherwise I don't see any of the other, possibly successfully-parsed, IDs.

nopcoder · 2020-10-12T22:21:05Z

parade/parade_test.go

+			if err := dst.Scan(c.in); err != nil {
+				if !errors.Is(err, c.err) {
+					t.Errorf("got err %v, expected %v", err, c.err)
+				}
+			} else {
+				if dst != c.out {
+					t.Errorf("expected %s, got %s", c.out, dst)
+				}
+			}


Suggested change

if err := dst.Scan(c.in); err != nil {

if !errors.Is(err, c.err) {

t.Errorf("got err %v, expected %v", err, c.err)

}

} else {

if dst != c.out {

t.Errorf("expected %s, got %s", c.out, dst)

}

}

err := dst.Scan(c.in)

if !errors.Is(err, c.err) {

t.Fatalf("got err %v, expected %v", err, c.err)

}

if err == nil && dst != c.out {

t.Fatalf("expected %s, got %s", c.out, dst)

}

Nice! Taking it but keeping it an error rather than a fatal. It gives me less indentation (which is good for my 80-column brain) and it tests the case where an error was expected but not received. Thanks!

codecov-io · 2020-10-13T12:08:04Z

Codecov Report

Merging #803 into master will increase coverage by 0.69%.
The diff coverage is 89.40%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   41.87%   42.57%   +0.69%     
==========================================
  Files         133      134       +1     
  Lines       10435    10583     +148     
==========================================
+ Hits         4370     4506     +136     
- Misses       5483     5491       +8     
- Partials      582      586       +4

Impacted Files	Coverage Δ
catalog/diff.go	`18.51% <ø> (ø)`
metastore/diff.go	`0.00% <0.00%> (ø)`
parade/ddl.go	`72.50% <87.50%> (+1.14%)`	⬆️
export/tasks_generator.go	`90.97% <90.97%> (ø)`
catalog/cataloger_commit.go	`81.81% <0.00%> (+3.03%)`	⬆️

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 1cea346...d7eb030. Read the comment docs.

arielshaqed

Thanks, @nopcoder ! PTAL...

arielshaqed · 2020-10-13T06:17:07Z

export/tasks_generator.go

+	"fmt"
+	"strings"
+
+	rinqueue "github.com/erikdubbelboer/ringqueue"


Auto-happened because of something Go-ish. There is a bug in ringqueue that the package is actually named rinqueue. Anything else causes fun with gofmt while saving etc.

arielshaqed · 2020-10-13T06:17:58Z

export/tasks_generator.go

+
+const successFilename = "_lakefs_success"
+
+const CopyActipn = "export:copy"


1+2 are great. Not doing 3, because Action is actually a string on TaskData.

arielshaqed · 2020-10-13T06:21:02Z

export/tasks_generator.go

+		dir = dirname(dir)
+		var ok bool
+		if ret, ok = dmc.upMatchCache[dir]; ok {
+			break
+		}
+		if dmc.pred(dir) {
+			copy := dir
+			ret = &copy
+			break
+		}
+		if dir == "" {
+			break
+		}
+	}
+	for dir = dirname(filename); dir != "" && (ret == nil || dir != *ret); dir = dirname(dir) {
+		dmc.upMatchCache[dir] = ret
+	}


Good point: it certainly does make sense to return true for the empty directory.

Exporting the object and writing a test for it, so I can fix its behaviour (in the sense of not changing, and also in the sense of making it behave correctly).

arielshaqed · 2020-10-13T07:10:50Z

export/tasks_generator.go

+	return *ret, true
+}
+
+func makeDirMatchCache(pred func(path string) bool) *dirMatchCache {


Value factories are called make in C++. Wait, you mean we're not using C++?!
(Done)

arielshaqed · 2020-10-13T12:02:19Z

export/tasks_generator.go

+// "generate success" task after generating all files in each directory that matches
+// generateSuccessFor.
+func GenerateTasksFromDiffs(exportID string, dstPrefix string, diffs catalog.Differences, generateSuccessFor func(path string) bool) ([]parade.TaskData, error) {
+	const initialSize = 100000


Not sure it matters, so reduced 100x.

arielshaqed · 2020-10-13T12:15:32Z

export/tasks_generator.go

+			return nil, fmt.Errorf("%s: failed to serialize %+v: %w", successPath, data, err)
+		}
+		bodyStr := string(body)
+		totalDependencies := td // copy to get new address each time


Huh? "I just want an lvalue reference...".

arielshaqed · 2020-10-13T12:16:35Z

parade/ddl.go

+var ErrBadTypeConversion = errors.New("bad type")
+
+// nolint: stylecheck
+func (dst *TaskStatusCodeValue) Scan(src interface{}) error {


I know. But the sql package does it this way

arielshaqed · 2020-10-13T12:17:16Z

parade/ddl.go

+		return fmt.Errorf("cannot convert %T to TaskStatusCodeValue: %w", src, ErrBadTypeConversion)
+	}
+
+	if !(sc == TaskPending || sc == TaskInProgress || sc == TaskAborted || sc == TaskCompleted) {


Not sure I understand what I need to do here.

arielshaqed · 2020-10-13T14:10:23Z

parade/parade_test.go

+	for rows.Next() {
+		var id parade.TaskID
+		if err = rows.Scan(&id); err != nil {
+			t.Errorf("[I] scan ID value: %s", err)


Why not? It's just a broken row (e.g. parse failure). rows.Next is welcome to fail if the database is 🔥 on fire.

So you're actually right, the code is wrong, and I added a test that rows were successfully scanned.

arielshaqed · 2020-10-13T14:11:25Z

parade/parade_test.go

+			if err := dst.Scan(c.in); err != nil {
+				if !errors.Is(err, c.err) {
+					t.Errorf("got err %v, expected %v", err, c.err)
+				}
+			} else {
+				if dst != c.out {
+					t.Errorf("expected %s, got %s", c.out, dst)
+				}
+			}


Nice! Taking it but keeping it an error rather than a fatal. It gives me less indentation (which is good for my 80-column brain) and it tests the case where an error was expected but not received. Thanks!

Correctly handle enum values and report errors nicely. This will give better behaviour when adding new enum values, and also makes code type-safer which is nice.

Only useful for tests, but still necessary for them.

These should *not* decrement total_dependencies, they've already been counted on num_signals.

This will be _partially_ filled in when computing differences, according to some requested set of fields. This will allow various applications of differences.

It's actually manually generated, and we missed it again on master.

Code was correct, but efficiency might not matter here.

Path is now Entry.Path.

This is a fairly major change to `DirMatchCache`, so explicitly test it (and capitalize `dirMatchCache` to export it...).

Turns out this is Go not C++, and constructors are not named like C++ value factories.

Remove dependency on `ringqueue`, which doesn't even name its package correctly. This might take a bit more memory, and/or be more CPU cache friendly. So it probably does not matter.

…slice

arielshaqed

Thanks!

Pulling without #795. @nopcoder ended up reviewing that too, so it doesn't necessarily need much further review. But it's still better off as a separate PR.

arielshaqed requested review from nopcoder and guy-har October 12, 2020 14:33

arielshaqed force-pushed the feature/534-diff-to-tasks branch from a4c1e0d to 61caf0d Compare October 12, 2020 14:35

nopcoder requested changes Oct 12, 2020

View reviewed changes

arielshaqed commented Oct 13, 2020

View reviewed changes

arielshaqed force-pushed the feature/534-diff-to-tasks branch from f3046b6 to 0cc7e51 Compare October 13, 2020 14:18

arielshaqed mentioned this pull request Oct 13, 2020

add export action and action manager #807

Merged

nopcoder approved these changes Oct 13, 2020

View reviewed changes

guy-har approved these changes Oct 14, 2020

View reviewed changes

arielshaqed added 16 commits October 14, 2020 09:13

Add explicit TaskStatusCodeValue handling on DB

495fa86

Correctly handle enum values and report errors nicely. This will give better behaviour when adding new enum values, and also makes code type-safer which is nice.

Handle num_signals column on database insert

8308282

Only useful for tests, but still necessary for them.

Extract function scanIDs in tests

039ea6a

Handle deleting aborted and completed tasks

7f23042

These should *not* decrement total_dependencies, they've already been counted on num_signals.

Add all of Entry to Difference

d284cea

This will be _partially_ filled in when computing differences, according to some requested set of fields. This will allow various applications of differences.

Add task generator for export operation

e406b8e

Create success tasks for nested directories

fc2b960

Post-merge broken autogenerated js/swagger.yml

ed47a62

It's actually manually generated, and we missed it again on master.

Silence checks warning: don't try to pass difference by reference

40643a7

Code was correct, but efficiency might not matter here.

Post-merge: update Difference Entry field

aaddf19

Path is now Entry.Path.

[CR] Fix CopyAction typo; use single const declaration

c9c93b2

[CR] Generate "success" file at top-level if required by predicate

5de8c62

This is a fairly major change to `DirMatchCache`, so explicitly test it (and capitalize `dirMatchCache` to export it...).

[CR] Rename constructor "make" -> "new"

b54114c

Turns out this is Go not C++, and constructors are not named like C++ value factories.

[CR] Traverse success dirs using LIFO collection (slice) not queue

842d5cd

Remove dependency on `ringqueue`, which doesn't even name its package correctly. This might take a bit more memory, and/or be more CPU cache friendly. So it probably does not matter.

[CR] Clean up: func comment, merge switch cases, reduce initial task …

0b076f5

…slice

[CR] improve and refine error reporting in tests

d7eb030

arielshaqed force-pushed the feature/534-diff-to-tasks branch from 0cc7e51 to d7eb030 Compare October 14, 2020 06:13

arielshaqed commented Oct 14, 2020

View reviewed changes

arielshaqed merged commit 6763612 into master Oct 14, 2020

arielshaqed deleted the feature/534-diff-to-tasks branch October 14, 2020 06:28

arielshaqed mentioned this pull request Oct 14, 2020

[parade] Allow deleting aborted and completed tasks #795

Closed


		const successFilename = "_lakefs_success"

		const CopyActipn = "export:copy"

	func makeDirMatchCache(pred func(path string) bool) *dirMatchCache {
	func newDirMatchCache(pred func(path string) bool) *dirMatchCache {

Generate tasks from diffs for exporting #803

Generate tasks from diffs for exporting #803

Conversation

arielshaqed commented Oct 12, 2020

arielshaqed commented Oct 12, 2020

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

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

codecov-io commented Oct 13, 2020 • edited Loading

Codecov Report

arielshaqed 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

arielshaqed left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 13, 2020 •

edited

Loading