Refactor delayed API responses #5955
Conversation
| // Check that JetStream is still on. Do not exit the go routine | ||
| // since JS can be enabled/disabled. The go routine will exit | ||
| // only if server is shutdown. | ||
| js := s.getJetStream() |
There was a problem hiding this comment.
The check for JS and the raft node quit channel, although working and the test checks that, may be a bit of an overkill. Does it really matter if a delayed response is sent or not after a raft group is shutdown or JS turned off? Again, code is there and test verifies it works, but we may have done without I think.
There was a problem hiding this comment.
I think it makes sense that we don't send anything back after the Raft group is stopped, otherwise it might be a signal for the client/application to "try again" rather than just timing out in the rate-limited scenario.
This is similar to @neilalexander PR (#5948) but has a slightly different approach. We use the struct to have a `next` so that we can use a list since the list will be maintained ordered. The for-loop is in one level, as opposed to two, which means that pushing to the IPQ will be "immediately" popped, even if we have a timer initialized and waiting to fire. The timer is reset (not recreated everytime) only if the added response ends-up being at the head of the list. Added a test that verifies that it works properly. Signed-off-by: Ivan Kozlovic <[email protected]>
neilalexander
force-pushed
the
delayed_api_responses
branch
from
13cfa7b
to
d6cfb6d
Compare
|
Rebased on top of |
server/jetstream_api.go
Outdated
| r *delayedAPIResponse | ||
| tm = time.NewTimer(time.Hour) | ||
| ) | ||
| setup := func() { |
There was a problem hiding this comment.
Minor nit but I suggest we call this function next instead of setup, and perhaps add a comment that r is the next response we're expecting the timer to fire for, updated only by calling next again.
There was a problem hiding this comment.
In fact I've just done this myself in a new commit.
There was a problem hiding this comment.
In one of my attempts, I had named it next(), so I would agree with that ;-).
Signed-off-by: Neil Twigg <[email protected]>
|
This one ready? |
This is similar to @neilalexander PR (#5948) but has a slightly different approach. We use the struct to have a
nextso that we can use a list since the list will be maintained ordered. The for-loop is in one level, as opposed to two, which means that pushing to the IPQ will be "immediately" popped, even if we have a timer initialized and waiting to fire.The timer is reset (not recreated everytime) only if the added response ends-up being at the head of the list.
Added a test that verifies that it works properly.
Signed-off-by: Ivan Kozlovic [email protected]