-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix panic in VTOrc #10519
Fix panic in VTOrc #10519
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good.
I'm just wondering if it is possible to write a test for this at all.
If that is too hard, can you confirm that this was verified via manual testing?
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
…t config Signed-off-by: Manan Gupta <manan@planetscale.com>
c8058bb
to
fd8f7c6
Compare
@deepthi Yes, when I found the problem, I jumped and fixed it without adding a reproducing test. I have fixed that now. There is a test which reproduces the panic (and fails on main), which passes after the changes! |
* test: reproduce the panic as a unit test Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: check ev is not nil before using its fields Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: increase timeout of LockShard and wait replicas in VTOrc default config Signed-off-by: Manan Gupta <manan@planetscale.com>
* test: reproduce the panic as a unit test Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: check ev is not nil before using its fields Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: increase timeout of LockShard and wait replicas in VTOrc default config Signed-off-by: Manan Gupta <manan@planetscale.com>
Description
A panic was seen in VTOrc with the following log -
On investigation it was found to occur because the
*events.Reparent
field returned was nil and the code does not check if the field is nil.This PR fixes this panic by checking if
ev
is nil before using its fields.This PR also increases the timeout of the default values of
LockShardTimeoutSeconds
andWaitReplicasTimeoutSeconds
to 30 seconds each.Related Issue(s)
Checklist
Deployment Notes