[chore] Schema Processor Revamp [Part 3] - Translation #37403
Conversation
| err = rev.all.Rollback(log) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = rev.logs.Rollback(log) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I see in ApplyAllResourceChanges the Revert case calls Rollback() funcs in the reverse order compared to the order the Update case calls Apply funcs. I can't tell immediately if it matters or no, but that order reversal intuitively sounds right to me.
Why are we not doing the same here (and elsewhere)? Is it important or no? Please make it consistent (and correct - if order matters). :-)
There was a problem hiding this comment.
It does matter! Nice catch, thanks
There was a problem hiding this comment.
The ordering still seems to be inconsistent. Here Revert does all, then logs. For spans Revert does spans, then all.
There was a problem hiding this comment.
This was an accident. I've double checked it and I think I fixed it all.
bd3c593 to
6a07e81
Compare
| err = rev.all.Rollback(log) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = rev.logs.Rollback(log) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The ordering still seems to be inconsistent. Here Revert does all, then logs. For spans Revert does spans, then all.
| ) | ||
|
|
||
| //go:embed testdata | ||
| var testdataFiles embed.FS |
There was a problem hiding this comment.
Nit: do we need to embed the test files or they can be just read as regular files?
There was a problem hiding this comment.
I'm not the most experienced go developer - is there any reason this is considered bad practice? I just thought it was convenient.
| ) | ||
| require.NoError(t, err, "Must not error when creating translator") | ||
|
|
||
| fixture.ParallelRaceCompute(t, 10, func() error { |
There was a problem hiding this comment.
Where in the current implementation do we have concurrency that needs race testing? I am probably missing it.
There was a problem hiding this comment.
I don't think there is any to be honest - a lot of this part's code is the contribution of @MovieStoreGuy so maybe he could speak as to his intention. Within this part, there's no concurrency. In the full processor, a lot of this code can be called under concurrent conditions (processors in general get called concurrently). The Manager piece gets called concurrently and has some logic around that.
c1bd4df to
ee7947e
Compare
Description
This is a slice of changes from #35248
This PR details Translation which defines the complete abstraction of schema translation file. It manages Revisions to move between two versions of a schema
Testing
Unit tests