Skip to content

Increment server failure on hostname lookup failures #8

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

Merged
merged 24 commits into from
Jul 30, 2025

Conversation

underwoo16
Copy link

@underwoo16 underwoo16 commented Jul 29, 2025

This change increments server failure counter when MEMCACHED_HOST_LOOKUP_FAILURE occurs

This will allow the server to be ejected when persistent DNS issues occur to prevent constant retries

Sample test failure before change:

memcached/test/unit/memcached_test.rb:1224:in `test_dns_nxdomain_server'
     1221:
     1222:     # Hit second server and pass the limit
     1223:     key2 = "test_missing_server_3"
  => 1224:     begin
     1225:       cache.get(key2)
     1226:     rescue => e
     1227:       assert_equal Memcached::ServerIsMarkedDead, e.class
<Memcached::ServerIsMarkedDead> expected but was
<Memcached::HostnameLookupFailure>

@Copilot Copilot AI review requested due to automatic review settings July 29, 2025 19:35
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a test case to verify that server ejection is not currently working for DNS errors when connecting to memcached servers. The test demonstrates that servers experiencing hostname lookup failures should be ejected but currently are not being properly marked as dead.

  • Adds a new test test_dns_nxdomain_server that exercises DNS error handling and server ejection
  • Removes Unix socket server from test configuration to simplify the test setup
  • Comments out UDP and domain socket memcached server initialization in setup

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/unit/memcached_test.rb Adds DNS error test case and updates server configuration to exclude Unix socket
test/setup.rb Comments out UDP and domain socket server initialization

@underwoo16 underwoo16 changed the title added test showing server ejection is not working for dns errors auto eject servers from dns errors Jul 29, 2025
@underwoo16 underwoo16 changed the title auto eject servers from dns errors Increment server failure on hostname lookup failures Jul 29, 2025
Copy link

@BrianGreenhill BrianGreenhill left a comment

Choose a reason for hiding this comment

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

I suspect it will be hard to roll this change gradually in prod so glad to see a thorough test here.

Can we try it in review lab first?

@srt32
Copy link
Member

srt32 commented Jul 30, 2025

Bad idea alert... ship the dep bump in a pr to review lab that also adds a non existent host to the mc config?

@underwoo16 underwoo16 merged commit 13d5237 into github Jul 30, 2025
0 of 4 checks passed
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.

3 participants