-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(lb) add req_dc_rr policy #114
Conversation
21b3209
to
15fd153
Compare
self.remote_tried = 0 | ||
if ngx and ngx.ctx and ngx.ctx.cassandra_coordinator then | ||
self.cassandra_coordinator = ngx.ctx.cassandra_coordinator | ||
end |
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.
Instead, I think we could do:
self.ctx = ngx and ngx.ctx
self.initial_cassandra_coordinator = self.ctx and self.ctx.cassandra_coordinator or nil
And in next_peer
, simply:
if i == 1 and state.initial_cassandra_coordinator then
end
-- ...
if state.cassandra_coordinator == peer then
end
if state.ctx then
state.ctx.cassandra_coordinator = peer
end
This way we avoid all of the global ngx
accesses and the metatable ones for ctx
t/14-lb_req_dc_rr.t
Outdated
ngx.say() | ||
for i, peer in lb:iter() do | ||
ngx.say("1.", i, ' ', peer.host) | ||
ngx.log(ngx.INFO, i, ' ', peer.host) |
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 seems like this log is unecessary
2.5 10.0.0.3 | ||
2.6 10.0.0.1 | ||
--- no_error_log | ||
[error] |
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.
Shouldn't we have a test that calls lb:iter()
once, then breaks, then calls it again, breaks again, then calls it again, and all 3 calls it should return the same peer? I don't see such a test, which I believe reflects the basic desired behavior for this new policy? The first test somewhat reflects that behavior, but does not break after each invocation, instead, it serves the last iteration's last returned peer as the first peer of the new iteration.
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.
Added the extra test, thanks for the review!
From #114 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Thanks! Manually merged along with a new |
No description provided.