REP-5470 Fix hang when handler thread hits an error. #86
Conversation
internal/verifier/check.go
Outdated
| if err = verifier.waitForChangeStream(ctx, csr); err != nil { | ||
| return errors.Wrapf( | ||
| err, | ||
| "failed to close %s", |
There was a problem hiding this comment.
How is the error related to closing a change stream reader?
| verifier.SetSrcNamespaces([]string{db.Name() + ".mycoll"}) | ||
| verifier.SetDstNamespaces([]string{db.Name() + ".mycoll"}) | ||
| verifier.SetNamespaceMap() | ||
| // verifier.SetVerifyAll(true) |
There was a problem hiding this comment.
why is this line commented out?
There was a problem hiding this comment.
This is a fairly large and complex diff for a codebase I don't understand very well. It looks okay, but there's a lot going on here. It seems like there's a lot more changes than the bare minimum needed to prevent the hang. Did you do some significant refactoring as well?
Would it be possible to split this up into multiple PRs?
internal/util/ctxutil.go
Outdated
| // | ||
| // NB: The returned error wraps both the context’s original error | ||
| // *and* the error’s cause. | ||
| func WrapCtxErrWithCause(ctx context.Context) error { |
There was a problem hiding this comment.
Is there any reason this isn't the same code as ctxutil.Err from mongo-go/ctxutil?
There was a problem hiding this comment.
None in particular; I can copy that over.
internal/verifier/change_stream.go
Outdated
| } | ||
| } | ||
|
|
||
| // This will prevent the reader from hanging. |
There was a problem hiding this comment.
I think it'd be good to say why it will prevent it from hanging.
I left a lot of debug logging in because they seem potentially useful and unlikely to pollute logs (excessively, anyhow).
No, actually. There’s some renaming (e.g.,
It seems a mite excessive for a <1k-line PR, most of whose changes are logging, but of course I wrote the changes. :) I can go back over this and make discrete commits. |
FGasper
force-pushed
the
REP-5470-ci-hangs
branch
from
c017079 to
6dc9081
Compare
|
@autarch I’ve separated this into discrete commits. |
I think the reason I'm struggling with this is that I don't particularly understand the state of the code before the PR. So this seems like a fairly large change to review in that context. I think discrete commits would make this a lot easier for me to review. I did take a look at the commit history but it didn't look amenable to a commit-by-commit review, which is part of why I asked if this could be broken up differently. |
This fixes a hang that previously happened if migration-verifier received an event that it couldn’t handle.
Specific changes:
This also removes a flapping assertion in
TestStartAtTimeNoChanges.