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
eds: fix priority timeout failure when EDS removes all priorities #3830
Conversation
…es all priorities Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.
Where does the panic happen? Should we rather fix the func passed to |
// Test the case where the first and only priority is removed. | ||
func (s) TestEDSPriority_FirstPriorityUnavailable(t *testing.T) { | ||
const testPriorityInitTimeout = time.Second | ||
defer func() func() { |
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.
Wow .... never seen this before defer func() func()
.
Cool
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.
It isn't much different from: assign new value, then defer func() to reassign old value back.
The only advantage is, it limits the scope of old
, so you can just name it old
, instead of oldPriorityInitTimeout
..
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.
defer func(t time.Duration) {
defaultPriorityInitTimeout = t
}(defaultPriorityInitTimeout)
defaultPriorityInitTimeout = testPriorityInitTimeout
?
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.
Done
clab2 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) | ||
edsb.handleEDSResponse(parseEDSRespProtoForTesting(clab2.Build())) | ||
|
||
// Wait after the init timer timeout, to ensure it doesn't fail. |
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.
Maybe wait for 2 times the value of testPriorityInitTimeout
to be double sure that the timeout has fired?
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.
Done
The panic happens here:
Maybe it should not panic, but should return a value. Will need to think what the correct value to return. |
Yeah, I also think that we should not panic from the |
Made For the other two ( |
…rities (grpc#3830) Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.
…s all priorities (grpc#3830) Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.
Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.