Skip to content
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

Issue #13: Updated route_failover to treat miss flag to return best on error. #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sergmour
Copy link

@sergmour sergmour commented Jan 4, 2025

Updated route_failover to treat miss flag to return best result on error (return miss if there were misses when requesting key from the list of children.
Issue: #13

@dormando
Copy link
Member

dormando commented Jan 4, 2025

Thanks! Bonus points if you add a test (it's not too hard to get running).

I probably copied mcrouter's behavior partially. I'll be back at a real computer next week to verify

@dormando
Copy link
Member

dormando commented Jan 4, 2025

#4 - oddly this sure sounds like the same report.

@dormando
Copy link
Member

dormando commented Jan 4, 2025

ah nm slightly different bug. bad response vs error response

@sergmour
Copy link
Author

sergmour commented Jan 4, 2025

I'll take a look on Monday and will add a test.

…rue (return miss-result if the last pool returns an error).
@dormando
Copy link
Member

dormando commented Jan 8, 2025

@sergmour any thoughts on tests? I'd like to get this fix in!

@sergmour
Copy link
Author

sergmour commented Jan 9, 2025

@dormando : Hey Dormando, I've just added a couple of tests. Sorry for the delay, the tests' setup was not straightforward for a person not familiar with lua. Note that the first test will fail for the older routelib.lua but will succeed for the updated one. If you agree with changes for route_failover, do you want me to extend this to the route_zfailover ?

return res
elseif res:ok() then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if res:hit() then
  return res
elseif res:ok()  then
  if miss == true then
    rmiss = res
  else
    return res
  end
else
  rerr = res
end

if rmiss then
  return rmiss
else
  return rerr
end

debating if this is more clear or not? I think it's also fewer branches?

thoughts?

@dormando
Copy link
Member

dormando commented Jan 9, 2025

thanks for the tests! Just one comment to see if we can simplify that big if or or or at the end

@dormando
Copy link
Member

dormando commented Jan 9, 2025

also sorry for the trouble getting the tests going. I'd thought I left directions but probably not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants