(VDB-1315) Add command for backfilling Urn diffs by rmulhol · Pull Request #189 · sky-ecosystem/vdb-mcd-transformers

rmulhol · 2020-05-07T03:34:47Z

insert diffs from every block with a frob for involved urn

paytonrules

This has been running in production for a while - so it's clearly working. However one thing I'd like to see would be some additions to the README explaining how to run these, and a hint on how to get started.

Nothing huge, just something like "we use Cobra for back-fills and command line scripts that run against VDB. Here's the command to run it. "

paytonrules · 2020-05-14T14:58:02Z

backfill/frob/backfiller.go

+
+	logrus.Infof("getting storage for %d grabs", len(frobs))
+	for i, frob := range frobs {
+		err := shared.FetchAndPersistDartDinkDiffs(frob.Dart, frob.Dink, frob.UrnID, frob.HeaderID, backFiller.eventsRepository, backFiller.storageRepository, backFiller.blockChain)


Maybe consider a struct for those first four params? It would prevent accidentally switching Dink/Dart and UrnID/HeaderID.

That's a great idea - I made this mistake earlier and manually rechecked these params approx ~1 million times 😂

paytonrules · 2020-05-14T15:00:10Z

backfill/grab/backfiller_test.go

+			Expect(errors.Is(err, fakes.FakeError)).To(BeTrue())
+		})
+
+		It("ignores grab if dink and dart are zero", func() {


Some of these tests are duplicated and could be moved to the Shared lib function that does most of the work.

paytonrules · 2020-05-14T15:02:47Z

backfill/frob/backfiller.go

+		}
+	}
+	return nil
+}


If you do another backfiller beyond these two, you may consider making each backfiller a strategy. Essentially add methods for things like "error message" and "log storage" and have one place that does the Get"Things" then loop through them logic. As opposed to copy/pasta.

paytonrules · 2020-05-14T15:38:40Z

backfill/repository/events_repository.go

+	err := e.db.Select(&grabs, `SELECT header_id, urn_id, dink, dart
+		FROM maker.vat_grab
+		JOIN public.headers ON vat_grab.header_id = headers.id
+		WHERE headers.block_number >= $1`, startingBlock)


How come you don't order the Grabs but do the Frobs? I'm not sure it matters but it's odd that they are slightly different.

Probably makes sense to order both 👍

The bigger the system gets and the more events we accumulate, I'm also starting to worry about unbounded results sets - i.e. how can we grab manageable chunks of events to backfill and proceed through from there

paytonrules · 2020-05-14T15:42:22Z

backfill/repository/events_repository_test.go

+			Expect(err).NotTo(HaveOccurred())
+			Expect(len(frobs)).To(Equal(2))
+			Expect(frobs[0]).To(Equal(frobAtEarlierBlock))
+		})


What about the error case? Does that require shutting down the db or something?

paytonrules · 2020-05-14T16:01:44Z

go.mod

 	github.com/onsi/gomega v1.7.0
 	github.com/pkg/errors v0.8.1 // indirect
 	github.com/sirupsen/logrus v1.2.0
+	github.com/spf13/cobra v0.0.3


- insert diffs from every block with a frob for involved urn

- Don't use create_back_filled_diff since it's slow and we don't need to guard against redundant diffs (e.g. same value at new block) since we're only attempting inserts when we know there's a delta

- Also rename to Frob Backfiller since we're fetching storage based on that event and may fetch urn data from other events

- Don't load all urns up front since it's likely only some of them have frobs since the starting block - Refactor to pull out shared code between frob + grab backfilling

elizabethengelman

LGTM!

Just a few questions/comments - nothing merge block though!

elizabethengelman · 2020-05-19T19:20:45Z

README.md

+This project contains one executable, `backfillUrns`.
+This command enables you to get storage data for Urns by performing lookups at blocks where we detected events indicating an Urn state change.
+To run it, you first need to `go build`.
+After that, you can run something like `./vdb-mcd-transformers backfillUrns --starting-block 10000000` - which would backfill Urn storage since block 10,000,000.


Super small, but I wonder if it would be worth mentioning that this commands backfills until the head of the chain?

elizabethengelman · 2020-05-19T19:36:43Z

backfill/repository/events_repository.go

+	return grabs, err
+}
+
+func (e eventsRepository) GetHeaderByID(id int) (core.Header, error) {


Wondering if this method would make sense in the HeaderRepository? 🤔

Yeah definitely - can open up a PR over there to add it 👍

elizabethengelman · 2020-05-19T19:49:04Z

backfill/repository/storage_repository.go

+	return urn, err
+}
+
+func (repo storageRepository) InsertDiff(diff types.RawDiff) error {


Was trying to think of a way to reuse the CreateStorageDiff code here, but it looks like we'd need to rework that method to allow a from_backfill value to be passed in so that we could make sure these are set to true. So maybe it's best to keep this as is, since it's specific to backfilling.

Will take a look - maybe we could add a new function in the storage repository, at least, for creating a back filled diff without using the DB function that protects against dupes

elizabethengelman · 2020-05-19T19:50:37Z

backfill/repository/storage_repository_test.go

+			urnErr := db.Get(&urnID, `INSERT INTO maker.urns (ilk_id, identifier) VALUES ($1, $2) RETURNING id`, ilkID, fakeUrn)
+			Expect(urnErr).NotTo(HaveOccurred())
+
+			urns, err := repo.GetUrnByID(urnID)


super small, but this could probably be urn instead of urns

elizabethengelman · 2020-05-19T20:03:34Z

backfill/shared/dart_dink.go

+	if err != nil {
+		return fmt.Errorf("error checking if dart/dink are zero: %w", err)
+	}
+	if dartIsZero && dinkIsZero {


I wonder if it's worth it to add a comment here about why we're returning when dart and dink are both zero?

I've been thinking about zero-valued diffs for too long and did a double take because I was thinking these were diffs, and not frob/grab values, so its also super possible this isn't necessary 😜

elizabethengelman · 2020-05-19T21:10:33Z

backfill/shared/keys.go

+	return storage.GetKeyForMapping(storage.IndexTwo, ilk)
+}
+
+func GetUrnArtKey(u repository.Urn) (common.Hash, error) {


I wonder if it's worth making getUrnArtKey and getUrnInkKey public so that we can reuse that code. If the way we get either of these keys changes, it would be nice to only have to change it in one place. Though I'm not sure how likely it is that that will ever change. 🤔

rmulhol requested review from elizabethengelman, paytonrules and yaoandrew May 7, 2020 03:34

rmulhol force-pushed the backfill-urns branch 5 times, most recently from aed69e6 to a0ea218 Compare May 12, 2020 16:58

paytonrules approved these changes May 14, 2020

View reviewed changes

Rob Mulholand added 5 commits May 15, 2020 15:53

Add command for backfilling Urn diffs

2c4e8fd

- insert diffs from every block with a frob for involved urn

Insert diff from urns_repository

72f7250

- Don't use create_back_filled_diff since it's slow and we don't need to guard against redundant diffs (e.g. same value at new block) since we're only attempting inserts when we know there's a delta

Add ilk art to backfiller

acc7779

- Also rename to Frob Backfiller since we're fetching storage based on that event and may fetch urn data from other events

Add support for back-filling based off of grab events

005e0dc

Get frobs before urns when backfilling

4b789fa

- Don't load all urns up front since it's likely only some of them have frobs since the starting block - Refactor to pull out shared code between frob + grab backfilling

rmulhol changed the title ~~Add command for backfilling Urn diffs~~ (VDB-1315) Add command for backfilling Urn diffs May 18, 2020

rmulhol force-pushed the backfill-urns branch from a0ea218 to bd5a199 Compare May 19, 2020 14:14

elizabethengelman approved these changes May 19, 2020

View reviewed changes

Address PR feedback

fafc8f5

rmulhol force-pushed the backfill-urns branch from bd5a199 to fafc8f5 Compare May 19, 2020 21:57

rmulhol merged commit 279c712 into staging May 19, 2020

rmulhol deleted the backfill-urns branch May 19, 2020 22:17

Conversation

rmulhol commented May 7, 2020

Uh oh!

paytonrules left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethengelman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants