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
Conversation
ashtul
commented
Dec 2, 2019
- Removed string not is place
- Added correct parsing for DD
- Tests
@ashtul you proably wanted to make this PR against the 1.4 branch |
src/pytest/test_replicate.py
Outdated
time.sleep(5) | ||
# Ensure slave is updated | ||
master.execute_command('set foo bar') | ||
try: |
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.
I don't see why we need try
here; it should just fail..
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.
I got the code from @MeirShpilraien.
I believe this will give a cleaner error message on the test.
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.
Mark I believe that a general timeout code is better then checking the time each iteration. Code looks cleaner.
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.
almost there!
found = True | ||
break | ||
time.sleep(0.01) | ||
|
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.
assertTrue(found)
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.
@mnunberg remove found all together
found = False | ||
break | ||
time.sleep(0.01) | ||
|
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.
assertFalse(found)
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.
remove found all together
src/pytest/test_replicate.py
Outdated
master.execute_command('ft.add', 'idx', 'doc%d' % i, 1.0, 'fields', | ||
'f', 'hello world') | ||
# Ensure slave is updated | ||
master.execute_command('set foo bar') |
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.
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 ...
src/pytest/test_replicate.py
Outdated
master.execute_command('ft.add', 'idx', 'doc%d' % i, 1.0, 'fields', | ||
'f', 'hello world') | ||
|
||
checkSlaveSynced(env, slave, ('ft.get', 'idx', 'doc0'), ['f', 'hello world']) |
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.
I would check the last document and not the first
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.
Small comment, other then this looks good..
@mnunberg @MeirShpilraien @gkorland |
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.
👍