Use TimerTask in the 3scale batcher policy #786
Conversation
davidor
changed the title
| local function ensure_report_timer_on(self, service_id, backend) | ||
| local check_timer = self.semaphore_report_timer:wait(0) | ||
| local function ensure_timer_task_created(self, service_id, backend) | ||
| local check_timer_task = self.semaphore_report_timer:wait(0) |
There was a problem hiding this comment.
Can we move this to .new ? This policy should not be initialized in init phase, right?
There was a problem hiding this comment.
We can't because we don't have access to the service ID in .new()
There was a problem hiding this comment.
Interesting. Maybe better to leave it for later, butI think we should think about changing the data structure to remove the service_id dependency. It should be as simple as create timer in new that gets reports from reports batcher. There should be no need for service_id because reports batcher instance is specific to this policy and timer, so it will ever contain only reports from this service. I see the reports batcher actually uses that service_id to do a lock on shmem. Hopefully we can solve that.
There was a problem hiding this comment.
You're right. That part can be simplified a lot.
There was a problem hiding this comment.
Could we generate some UUID instead of using service id ? Then every policy would take care just of its own data.
There was a problem hiding this comment.
This is related to an idea that we discussed some time ago.
Right now, pending reports are stored in a shared dictionary and every instance of the policy creates a timer (per worker) to report all the pending reports for its service ID.
After introducing TimerTask I see it more clearly that it might be better to adopt a different strategy. We could store the pending reports in a table of the instance, as we know that there'll only be one instance of the policy for a given service ID per Apicast worker. Pros: no locks, simpler code. Cons: possibility of losing reports if a worker dies, which in practice I don't think it'll be an issue.
There was a problem hiding this comment.
Let's leave this for a future PR.
In this one, I'd like to focus on switching to using TimerTask to be able to cancel timers.
davidor
force-pushed
the
use-timertasks-in-batcher-policy
branch
6 times, most recently
from
11ccd3d to
511cce3
Compare
davidor
force-pushed
the
use-timertasks-in-batcher-policy
branch
from
511cce3 to
bc4c74a
Compare
davidor
changed the title
davidor
changed the title
davidor
force-pushed
the
use-timertasks-in-batcher-policy
branch
2 times, most recently
from
0b1cddd to
74ba6b1
Compare
davidor
changed the title
| local usage = context.usage | ||
| local service = context.service | ||
| local service_id = service.id | ||
| self.service_id = service_id |
There was a problem hiding this comment.
Why it is being assigned to self ?
There was a problem hiding this comment.
Sorry, this is a leftover from some test. Same with self.backend.
| function _M:schedule_one(delay) | ||
| -- Need to wrap the task in a function that discards the first param (the | ||
| -- "premature" one sent by ngx.timer.at) | ||
| ngx.timer.at(delay or 0, function(_, ...) self.task(...) end, unpack(self.args)) |
There was a problem hiding this comment.
This is going to allocate a function for each call. Is that necessary?
There was a problem hiding this comment.
Does no longer apply. This method has been replaced.
|
|
||
| -- Can't check all the arguments of ngx.timer.at because it calls an | ||
| -- private function but at least we can check the interval (first arg) | ||
| assert.equals(interval, ngx_timer_stub.calls[1].vals[1]) |
There was a problem hiding this comment.
I think you can change this to use matchers and verify it was called with some function (matcher.function) and exact second parameter.
There was a problem hiding this comment.
But the function is private, so I don't think we can do that.
There was a problem hiding this comment.
Matcher can match the type like:
assert.spy(ngx_timer_stub).was_called_with(matcher.is_function(), 1)There was a problem hiding this comment.
I'm familiar with matchers, but I misunderstood you.
I thought that you wanted to check that ngx_timer_stub was called with a specific function and I was saying that it was not possible because it's a private one. Now I see that you suggested just to check that it's a function no matter which one.
There was a problem hiding this comment.
I added a check for this 👍
|
|
||
| if self.timer_task then | ||
| self.timer_task:schedule_one(1) | ||
| end |
There was a problem hiding this comment.
And we will rely on cancelling the timer by __gc callbacks ?
There was a problem hiding this comment.
That works, but let's cancel the timer_task here to be explicit about it.
| -- run before that so we do not leave any pending reports. | ||
|
|
||
| if self.timer_task then | ||
| self.timer_task:schedule_one(1) |
There was a problem hiding this comment.
:)
From the point of view of the policy, I think it does not matter. However, busted crashes with a segfault when this is 0. Not sure why exactly. It might be related to how busted collects the garbage after all the tests. It does not crash with a delay of 0.1.
There was a problem hiding this comment.
Ok, then this is definitely worth a comment :-D
There was a problem hiding this comment.
In the end I changed this and implemented it as we discussed off-line: ba883c1
davidor
force-pushed
the
use-timertasks-in-batcher-policy
branch
2 times, most recently
from
f3d949e to
5398758
Compare
Unfortunately, the specs were mocking ReportsBatcher and that was hiding a bug. ReportsBatcher:add() was not being called with the correct parameters.
…er to real usage Before, the tests set a high reporting interval so a report was not triggered in the middle of the test. The tests relied on calling a specific endpoint to force a report to backend. Now, instead of doing that, we let the policy to run report jobs as it would normally do, we aggregate the data we receive in the reporting endpoint, and at the end of the test, we verify it. This approach is closer to a real usage of the policy.
davidor
force-pushed
the
use-timertasks-in-batcher-policy
branch
from
5398758 to
b04d254
Compare
This PR replaces the usages of
ngx.timer.everywithTimerTaskinstances.This solves the issue of not being able to cancel timers. Recurrent tasks created using
TimerTaskcan be canceled, but we cannot cancel timers created withngx.timer.every. This was causing a bug, Timer would continue running even after a config reload.