scdeps: tighten dependencies, log more side effects #73873
Conversation
|
This change is |
postamar
force-pushed
the
more-side-effects
branch
from
dacc424
to
e126968
Compare
|
@ajwerner this is the PR that relates to ajwerner@9d0010a |
postamar
force-pushed
the
more-side-effects
branch
from
e126968
to
c2c6557
Compare
| for _, statementID := range statementIDs { | ||
| entries := eventLogEntriesForStatement(mvs.eventsByStatement[statementID]) | ||
| for _, e := range entries { | ||
| if err := deps.EventLogger().LogEvent(ctx, e.id, *e.metadata, e.event); err != nil { |
There was a problem hiding this comment.
It'd be nice to make this interface and call batched. We're pretty close to having that all line up. Happy to defer to a later PR. Thoughts on a TODO here or on the interface?
There was a problem hiding this comment.
Good call. I'll do what I can.
| @@ -143,10 +145,11 @@ commit transaction #1 | |||
| # begin PostCommitPhase | |||
| begin transaction #2 | |||
| ## PostCommitPhase non-revertible stage 1 of 1 with 11 MutationType ops | |||
| write *eventpb.DropSchema to event log for descriptor #57: DROP SCHEMA db.sc CASCADE | |||
There was a problem hiding this comment.
Should we pull more structure out of this entry? How verbose would it be to print the whole entry?
There was a problem hiding this comment.
At some point we should. Right now, adding more detail doesn't seem useful. I'm happy to be convinced otherwise, though.
| sourceEvent := sourceEvents[subWorkID] | ||
| switch sourceEvent.event.(type) { | ||
| case *eventpb.DropDatabase: | ||
| // Drop database only reports dependent schemas. |
There was a problem hiding this comment.
This feels so arbitrary. Is there some principle here? Do all dropped descriptors end up getting captured but I'm just missing how? Maybe printing the complete events in the end-to-end test will make it more obvious.
There was a problem hiding this comment.
I have a feeling that Faizan is working on this right now.
This commit reworks the dependency injection for the event logger, among other declarative schema changer dependencies. It also makes the test dependencies more chatty in the side effects log. Release note: None
postamar
force-pushed
the
more-side-effects
branch
from
c2c6557
to
b986c48
Compare
|
Thanks for the review! bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This commit reworks the dependency injection for the event logger, among
other declarative schema changer dependencies. It also makes the test
dependencies more chatty in the side effects log.
Release note: None