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.
Wow .... never seen this before defer func() func().
Cool
There was a problem hiding this comment.
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.
defer func(t time.Duration) {
defaultPriorityInitTimeout = t
}(defaultPriorityInitTimeout)
defaultPriorityInitTimeout = testPriorityInitTimeout?
| 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.
Maybe wait for 2 times the value of testPriorityInitTimeout to be double sure that the timeout has fired?
|
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.