Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Plug hole that allowed concurrent replication streams #323

Merged
merged 4 commits into from
Nov 20, 2017
Merged

Plug hole that allowed concurrent replication streams #323

merged 4 commits into from
Nov 20, 2017

Conversation

kirg
Copy link
Contributor

@kirg kirg commented Nov 15, 2017 •
edited
Loading

  • fix for concurrent replication streams
  • update the 'first seqnum' in replication code-path (this was missing)
  • added more logs around open/close extent

@kirg kirg changed the title Do not allow more than one replication Concurrent re[plication bug-fix Nov 15, 2017
@kirg kirg changed the title Concurrent re[plication bug-fix Concurrent replication bug-fix Nov 15, 2017
kirg
kirg commented Nov 15, 2017
View reviewed changes
services/storehost/extMgr.go Outdated

if openError != nil {
// mark as not open for replication
atomic.StoreInt32(&ext.openedForReplication, 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ the fix

Copy link
Contributor

Choose a reason for hiding this comment

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

if openError == nil?

@coveralls
Copy link

coveralls commented Nov 15, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 66.936% when pulling f7189ba on storehost-replication into 65a1bb1 on master.

@kirg kirg changed the title Concurrent replication bug-fix Plug hole that allowed concurrent replication streams Nov 15, 2017
@thuningxu
Copy link
Contributor

Can you add unit test?

@kirg
Copy link
Contributor Author

kirg commented Nov 17, 2017

There is a unit-test that checks to see that when two OpenReplicateStream requests come in, only one will succeed and the other will fail. Unfortunately, this bug shows up when a third OpenReplicateStream request comes in!

@kirg kirg mentioned this pull request Nov 17, 2017
Closed
@coveralls
Copy link

coveralls commented Nov 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 66.895% when pulling d1b5c4d on storehost-replication into 65a1bb1 on master.

thuningxu
thuningxu previously approved these changes Nov 18, 2017
View reviewed changes
@thuningxu thuningxu dismissed their stale review November 18, 2017 18:35

Waiting for unit test

@coveralls
Copy link

coveralls commented Nov 19, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 66.763% when pulling 2ef7258 on storehost-replication into 65a1bb1 on master.

@kirg
Copy link
Contributor Author

kirg commented Nov 19, 2017

@thuningxu I have updated the existing test to try another (third) replicate-extent request, which should fail; verified that it fails without fix and passes with.

thuningxu
thuningxu approved these changes Nov 20, 2017
View reviewed changes
Copy link
Contributor

@thuningxu thuningxu left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks!

@kirg kirg merged commit fb33a45 into master Nov 20, 2017
@kirg kirg deleted the storehost-replication branch November 20, 2017 02:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thuningxu thuningxu thuningxu approved these changes

@datoug datoug Awaiting requested review from datoug

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kirg @coveralls @thuningxu