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

Conversation

ashtul
Copy link
Contributor

@ashtul ashtul commented Dec 2, 2019

  • Removed string not is place
  • Added correct parsing for DD
  • Tests

@mnunberg
Copy link
Contributor

mnunberg commented Dec 2, 2019

@ashtul you proably wanted to make this PR against the 1.4 branch

@mnunberg mnunberg changed the base branch from master to 1.4 December 2, 2019 16:27
src/pytest/test_replicate.py Outdated Show resolved Hide resolved
time.sleep(5)
# Ensure slave is updated
master.execute_command('set foo bar')
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.

src/pytest/test_replicate.py Show resolved Hide resolved
src/pytest/test_replicate.py Outdated Show resolved Hide resolved
@ashtul ashtul requested a review from mnunberg December 3, 2019 09:28
Copy link
Contributor

@mnunberg mnunberg left a 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)

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

found = False
break
time.sleep(0.01)

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

mnunberg
mnunberg previously approved these changes Dec 3, 2019
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')
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 ...

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'])
Copy link
Collaborator

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

Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a 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..

@ashtul
Copy link
Contributor Author

ashtul commented Dec 4, 2019

@mnunberg @MeirShpilraien @gkorland
Mark's earlier approval still holds even after new commit

Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍

@ashtul ashtul merged commit f365a73 into 1.4 Dec 4, 2019
@ashtul ashtul deleted the issue993-ft.del-RedisModule_Replicate branch December 4, 2019 11:38
@emmanuelkeller emmanuelkeller added this to the 1.4.19 milestone Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants