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

Issue993 FT.DEL redismodule_replicate #1000

Merged
merged 12 commits into from Dec 4, 2019
46 changes: 39 additions & 7 deletions src/pytest/test_replicate.py
@@ -1,23 +1,48 @@
import subprocess
import signal
import os
import os.path
from RLTest import Env
import time

class TimeLimit(object):
"""
A context manager that fires a TimeExpired exception if it does not
return within the specified amount of time.
"""
def __init__(self, timeout):
self.timeout = timeout
def __enter__(self):
signal.signal(signal.SIGALRM, self.handler)
ashtul marked this conversation as resolved.
Show resolved Hide resolved
signal.setitimer(signal.ITIMER_REAL, self.timeout, 0)
def __exit__(self, exc_type, exc_value, traceback):
signal.setitimer(signal.ITIMER_REAL, 0)
signal.signal(signal.SIGALRM, signal.SIG_DFL)
def handler(self, signum, frame):
raise Exception()

def testDelReplicate():
env = Env(useSlaves=True)
master = env.getConnection()
slave = env.getSlaveConnection()
slave = env.getSlaveConnection()

env.assertContains("PONG", master.execute_command("ping"))
env.assertContains("PONG", slave.execute_command("ping"))

env.assertOk(master.execute_command('ft.create', 'idx', 'schema', 'f', 'text'))

for i in range(10):
master.execute_command('ft.add', 'idx', 'doc%d' % i, 1.0, 'fields',
'f', 'hello world')

time.sleep(5)
# Ensure slave is updated
master.execute_command('set foo bar')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the correct way to check it, when we move the replicate to the indexer thread the set might get to the slave before the ft.add command. The correct way is try to get the document from the slave with timeout as I suggested from the first place ...

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we need try here; it should just fail..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the code from @MeirShpilraien.
I believe this will give a cleaner error message on the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mark I believe that a general timeout code is better then checking the time each iteration. Code looks cleaner.

with TimeLimit(5):
res = slave.execute_command('get foo')
env.assertEqual('bar', res)
while len(res) != 3:
res = slave.execute_command('get foo')
except Exception:
env.assertTrue(False, message='Failed waiting for registration to unregister on slave')

Copy link
Contributor

Choose a reason for hiding this comment

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

assertTrue(found)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mnunberg remove found all together

for i in range(10):
# checking for insertion
Expand All @@ -30,12 +55,19 @@ def testDelReplicate():
env.assertEqual(1, master.execute_command(
'ft.del', 'idx', 'doc%d' % i, 'DD'))

time.sleep(5)
# Ensure slave is updated
master.execute_command('set foo baz')
try:
with TimeLimit(5):
res = slave.execute_command('get foo')
while len(res) == 'bar':
res = slave.execute_command('get foo')
except Exception:
env.assertTrue(False, message='Failed waiting for registration to unregister on slave')

Copy link
Contributor

Choose a reason for hiding this comment

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

assertFalse(found)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove found all together

for i in range(10):
# checking for deletion
env.assertEqual(None,
master.execute_command('ft.get', 'idx', 'doc%d' % i))
env.assertEqual(None,
slave.execute_command('ft.get', 'idx', 'doc%d' % i))

slave.execute_command('ft.get', 'idx', 'doc%d' % i))