Vdb 1528 decouple diff transforming#131
Conversation
6fe54b3 to
9502d15
Compare
9502d15 to
36a88fc
Compare
rmulhol
left a comment
There was a problem hiding this comment.
LGTM!
Main thing I'm noodling on is how to manage multiple execute processes in the future (e.g. if we also want to look at unwatched diffs)
cmd/execute.go
Outdated
| go watchEthStorage(&sw, &wg) | ||
| } | ||
|
|
||
| if len(ethStorageInitializers) > 0 { |
There was a problem hiding this comment.
pretty minor but we could probably combine these operations if they have the same predicate, right?
cmd/execute.go
Outdated
| } | ||
|
|
||
| if len(ethStorageInitializers) > 0 { | ||
| storageHealthCheckMessage := []byte("storage watcher for unrecognized diffs starting\n") |
There was a problem hiding this comment.
This will probably need to happen in a separate PR against vdb-mcd-transformers (and get updated by tech ops in the deploy), but I imagine we'll want to update the health check to check for both messages
There was a problem hiding this comment.
I think that the transformers PR should take care of checking for the both message: https://github.com/makerdao/vdb-mcd-transformers/pull/277/files#diff-015ae56a7505cbe6e703b68709ad40e1. I'm not sure what we would need to have tech ops change though 🤔
cmd/root.go
Outdated
| SubCommand string | ||
| cfgFile string | ||
| databaseConfig config.Database | ||
| newDiffBlockFromHeadOfChain int64 |
There was a problem hiding this comment.
would it make sense to declare this with unrecognizedDiffBlockFromHeadOfChain?
There was a problem hiding this comment.
Yeah, good call. I had put it in the execute cmd file, but having it in root.go would be more consistent.
| StorageDiffRepository: storageDiffRepository, | ||
| DiffBlocksFromHeadOfChain: backFromHeadOfChain, | ||
| StatusWriter: statusWriter, | ||
| DiffStatus: diffStatusToWatch, |
There was a problem hiding this comment.
not sure if this would make things less clear but I wonder if we could directly pass the function that should be called to get diffs, instead of a flag
There was a problem hiding this comment.
🤔 I really like this idea. I may hold off, and do it as part of a new PR/story. I think it will require a bit of reworking since the GetNewDiffs and GetUnrecognizedDiffs are methods on the diffRepository which the execute command doesn't know about. Open to thoughts on how to get around this without a bunch of reworking how the repository works!
36a88fc to
b1beab2
Compare
companion transformers PR: sky-ecosystem/vdb-mcd-transformers#277