Refactor LoadLayer - #99
Conversation
7adfe21 to
73c3b0c
Compare
… error handling and tracking. Fix test_codegen scenario
| singleEntities: dict<array<loadSingle<Entities.internalEntity>>>, | ||
| lookupByIndex: dict<loadIndex<Entities.internalEntity>>, | ||
| entityMod: module(Entities.InternalEntity), | ||
| logger: Pino.t, |
There was a problem hiding this comment.
The problem with hoisting the logger up is that logger actually contains a context for the logging. So in other words if we hand down the logger from a given handler/loader it will contain info about which event was being handled on the actual log. So it's probably better to put it on the action
There was a problem hiding this comment.
Hmm, I'll think about it
It's pretty important for debugging that when you get a log/error it prints out info of which event it came from on which chain
|
In the two PRs above:
Incoming:
|
a886d0c to
ce0b0b4
Compare
| let promise = Promise.make((resolve, reject) => { | ||
| let loadRecord: loadSingle = { | ||
| resolve, | ||
| reject, | ||
| promise: %raw(`null`) | ||
| } | ||
| map.byId->Js.Dict.set(entityId, loadRecord) | ||
| }) | ||
| // Don't use ref, since it'll allocate an object to store .contents | ||
| (map.byId->Js.Dict.unsafeGet(entityId)).promise = promise |
There was a problem hiding this comment.
I made something very similar in utils FYI. (Called promiseWithHandles I think)
There was a problem hiding this comment.
I've checked it out, but decided to remove promiseWithHandles instead. I found it less performant, and since it's not used anywhere, I thought that it's better not to have the abstraction.
|
I fixed all your comments besides Also, for the case when we load by ids, that's a single operation while every id can have their own logger context. Hence, I think having a more general context which doesn't contain a specific |
| ) => promise<array<Entities.internalEntity>>, | ||
| makeLoadEntitiesByField: ( | ||
| ~entityMod: module(Entities.InternalEntity), | ||
| . ~entityMod: module(Entities.InternalEntity), |
There was a problem hiding this comment.
No, but for some reason vscode extention started to formatting the file using the curried mode setting on. So to prevent breaking it, I've decided to add . here.
| }) | ||
|
|
||
| Async.it( | ||
| "Correctly gets entity from inMemoryStore by index if the entity set after the index creation", |
There was a problem hiding this comment.
#105 This should change the outcome of this test right?
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Awesome stuff Dmitry! The tests are really clear and nice. And I'm super stoked about the refactor.
Really amazing addition to the indexer 👏🏼 👏🏼
2 participants
Refactoring the Loader Layer in the Indexer
Overview
Link to Loom
PR presentation:
https://www.loom.com/share/b6e4e83cdecc4bc0a4f9b12b6d628037
Explaining the changes:
https://www.loom.com/share/a0cb74cc2c4d4e4ab92a76c990fa3384