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

pkg/loader: fix loader run while quitting #749

Merged
merged 7 commits into from
Sep 17, 2019

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Sep 11, 2019

What problem does this PR solve?

TOOL-1579
When loader meets some errors in loader.Run it will quit immediately. But if there is some other functions trying to syncing txns to loader.input they may get blocked. This problem is similar to #714.

What is changed and how it works?

Add select in mysql dsyncer. When loader finished loader.run, mysql.Error() will be activated which can avoid block.

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

Related changes

@lichunzhu
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

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

When loader.Run stopped, MySQLSyncer would call setErr, and the errCh channel would be closed regardless of whether the passed error is nil or not.
So maybe we can simply wrap the blocking loader.Input() <- with a select like the following to solve the problem.

diff --git a/drainer/sync/mysql.go b/drainer/sync/mysql.go
index c9067b3e..10343e3e 100644
--- a/drainer/sync/mysql.go
+++ b/drainer/sync/mysql.go
@@ -83,9 +83,12 @@ func (m *MysqlSyncer) Sync(item *Item) error {
 
 	txn.Metadata = item
 
-	m.loader.Input() <- txn
-
-	return nil
+	select {
+	case <-m.errCh:
+		return m.err
+	case m.loader.Input() <- txn:
+		return nil
+	}
 }
 
 // Close implements Syncer interface

@lichunzhu
Copy link
Contributor Author

@suzaku Nice suggestion! It works for MysqlSyncer. But the farther developers may make the same mistakes again. Actually I think it's very easy to make mistakes here if some other developers want to reuse the loader.Input code. So shall we leave some comments or other things?

@suzaku
Copy link
Contributor

suzaku commented Sep 11, 2019

One possible way is to encapsulate the enqueueing of txns with a method that may return an error if the loader's not in a consistent state.
So that users don't have to remember to use select when sending to the input channel.

Maybe we can do that in a separate PR.

For this problem, can we add a test case to prove that we have solved the hanging problem?

@lichunzhu
Copy link
Contributor Author

/run-integration-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

drainer/sync/mysql_test.go Outdated Show resolved Hide resolved
drainer/sync/mysql_test.go Outdated Show resolved Hide resolved
drainer/sync/mysql_test.go Outdated Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

/run-all-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu merged commit b8adb11 into pingcap:master Sep 17, 2019
@lichunzhu lichunzhu deleted the czli/fixLoaderQuit branch September 17, 2019 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants