Skip to content

Commit 95c4ee0

Browse files
John Huggmeta-codesync[bot]
authored andcommitted
Add Thrift-only real UCache test fixture
Summary: Part of the effort to remove UCache's legacy ASCII/Caret serving path. Enable the migration away from UCache ASCII/Caret serving to be validated against a real process with no legacy listener. Keep the mode opt-in so shared protocol coverage remains unchanged, and require typed Thrift readiness so the fixture cannot report success without checking an endpoint. The thrift-only `MCProcess` mode (`connectLegacySocket=False`) validates its inputs at construction: it requires a Thrift port and the Thrift test client, and rejects `versionPing` (which needs a legacy socket). Reviewed By: ghostonhuang Differential Revision: D113456755 fbshipit-source-id: 788401f2a152213500d3e021ba17336f56331633
1 parent 0b28dc9 commit 95c4ee0

2 files changed

Lines changed: 60 additions & 22 deletions

File tree

mcrouter/test/MCProcess.py

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,22 @@ def __init__(
143143
use_ssl=False,
144144
versionPing=False,
145145
thriftPort=None,
146+
connectLegacySocket=True,
146147
):
148+
if not connectLegacySocket:
149+
if thriftPort is None:
150+
raise ValueError("A Thrift port is required without a legacy socket")
151+
if not McrouterGlobals.useThriftClient():
152+
raise ValueError(
153+
"The Thrift test client must be enabled without a legacy socket"
154+
)
155+
if versionPing:
156+
raise ValueError(
157+
"versionPing requires a legacy socket and is unsupported without one"
158+
)
147159
self.fd = None
148160
self.versionPing = versionPing
161+
self.connectLegacySocket = connectLegacySocket
149162
if cmd is not None and "-s" in cmd:
150163
if os.path.exists(addr):
151164
raise Exception(f"file path {addr} already exists")
@@ -228,27 +241,28 @@ def connect(self):
228241

229242
def ensure_connected(self):
230243
retry_count = 0
231-
# First, try to connect
232-
while True:
233-
try:
234-
self.connect()
235-
break
236-
except Exception as e:
237-
retry_count += 1
238-
print(
239-
# pyrefly: ignore [missing-attribute]
240-
f"Cannot connect (errno: {e.errno}). Retry {retry_count} of {self.max_retries}."
241-
)
242-
self.disconnect()
243-
if not self.is_alive():
244-
print("Process exited unexpectedly!")
245-
self.terminate() # This will print logs as well
244+
if self.connectLegacySocket:
245+
# First, try to connect
246+
while True:
247+
try:
248+
self.connect()
249+
break
250+
except Exception as e:
251+
retry_count += 1
252+
print(
253+
# pyrefly: ignore [missing-attribute]
254+
f"Cannot connect (errno: {e.errno}). Retry {retry_count} of {self.max_retries}."
255+
)
256+
self.disconnect()
257+
if not self.is_alive():
258+
print("Process exited unexpectedly!")
259+
self.terminate() # This will print logs as well
260+
raise
261+
# If we defined a retry count, retry until that's exceeded.
262+
if not self.max_retries or retry_count < self.max_retries:
263+
time.sleep(1)
264+
continue
246265
raise
247-
# If we defined a retry count, retry until that's exceeded.
248-
if not self.max_retries or retry_count < self.max_retries:
249-
time.sleep(1)
250-
continue
251-
raise
252266

253267
# Then, verify Memcache is ready for traffic
254268
if self.versionPing:

mcrouter/tests/test_mcprocess.py

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,22 @@
99
from unittest.mock import Mock, patch
1010

1111
from carbon.carbon_result.thrift_types import Result
12-
from mcrouter.test.MCProcess import MCProcess
12+
from mcrouter.test.MCProcess import MCProcess, McrouterGlobals
1313

1414

15-
class TestMCProcessEnsureConnected(unittest.TestCase):
15+
class MCProcessEnsureConnectedTest(unittest.TestCase):
16+
@patch.object(McrouterGlobals, "useThriftClient", return_value=False)
17+
def test_thrift_only_requires_enabled_thrift_client(self, use_thrift_client):
18+
with self.assertRaisesRegex(ValueError, "Thrift test client must be enabled"):
19+
MCProcess(
20+
cmd=None,
21+
addr=12345,
22+
thriftPort=12346,
23+
connectLegacySocket=False,
24+
)
25+
26+
use_thrift_client.assert_called_once_with()
27+
1628
def make_process(self, thrift_client=None):
1729
# Bypass __init__ to isolate ensure_connected: construct a bare MCProcess
1830
# and set only the attributes the readiness path reads. If __init__ later
@@ -23,6 +35,7 @@ def make_process(self, thrift_client=None):
2335
process.is_alive = Mock(return_value=True)
2436
process.terminate = Mock()
2537
process.versionPing = False
38+
process.connectLegacySocket = True
2639
process.thrift_client = thrift_client
2740
process.max_retries = 4
2841
return process
@@ -42,6 +55,17 @@ def test_thrift_readiness_retries_until_ok(self, sleep):
4255
self.assertEqual(3, thrift_client.mcVersion.call_count)
4356
self.assertEqual(2, sleep.call_count)
4457

58+
def test_thrift_only_readiness_skips_legacy_connection(self):
59+
thrift_client = Mock()
60+
thrift_client.mcVersion.return_value = SimpleNamespace(result=Result.OK)
61+
process = self.make_process(thrift_client)
62+
process.connectLegacySocket = False
63+
64+
process.ensure_connected()
65+
66+
process.connect.assert_not_called()
67+
thrift_client.mcVersion.assert_called_once_with()
68+
4569
@patch("mcrouter.test.MCProcess.time.sleep")
4670
def test_child_exit_during_thrift_readiness_fails_promptly(self, sleep):
4771
error = ConnectionError("listener unavailable")

0 commit comments

Comments
 (0)