Avoid a data race when dealing with the lowwatermark (#1530)

* Remove error return value since we don't use it.

* Lock the mutex whenever we plan to update the low watermark to avoid a race condition.

* Check for data races in our unit tests.

* Still return an error from ProcessEventsUntilDrained but actually check it in our code.

* Make coordinator_test.go to check the err from  ProcessEventsUntilDrained again

* Remove unreachable return in ProcessEventsUntilDrained

Author: misalcedo

go/logic/coordinator.go

@@ -377,9 +377,11 @@ func (c *Coordinator) ProcessEventsUntilDrained() error {
 
 				switch binlogEvent := ev.Event.(type) {
 				case *replication.GTIDEvent:
+					c.mu.Lock()
 					if c.lowWaterMark == 0 && binlogEvent.SequenceNumber > 0 {
 						c.lowWaterMark = binlogEvent.SequenceNumber - 1
 					}
+					c.mu.Unlock()
 				case *replication.RotateEvent:
 					c.currentCoordinatesMutex.Lock()
 					c.currentCoordinates.LogFile = string(binlogEvent.NextLogName)

go/logic/migrator.go

@@ -1319,7 +1319,9 @@ func (this *Migrator) executeWriteFuncs() error {
 
 		// We give higher priority to event processing.
 		// ProcessEventsUntilDrained will process all events in the queue, and then return once no more events are available.
-		this.trxCoordinator.ProcessEventsUntilDrained()
+		if err := this.trxCoordinator.ProcessEventsUntilDrained(); err != nil {
+			return this.migrationContext.Log.Errore(err)
+		}
 
 		this.throttler.throttle(nil)
 

script/test

@@ -14,4 +14,4 @@ script/build
 cd .gopath/src/github.com/github/gh-ost
 
 echo "Running unit tests"
-go test -v -p 1 -covermode=atomic ./go/...
+go test -v -p 1 -covermode=atomic -race ./go/...