Made InlineVerifier resumable #127

shuhaowu · 2019-08-26T20:48:08Z

This PR makes all of Ghostferry interrupt/resumable when running with the InlineVerifier. Most of the code are the ruby integration tests.

Also note: Previously, RunStandaloneCopy was recording improper LastSuccessfulPK position as the BatchWriter from Ferry is reused in it. This is fixed. Furthermore, the IterativeVerifier is removed from deltaCopyJoinedTables and copyPrimaryKeyTables. Instead, RunStandaloneCopy now automatically verifies the data via the InlineVerifier.

Previously, RunStandaloneCopy was recording improper LastSuccessfulPK position as the BatchWriter is reused. This is fixed. Furthermore, the IterativeVerifier is removed from deltaCopyJoinedTables and copyPrimaryKeyTables. Instead, RunStandaloneCopy now automatically verifies the data via the InlineVerifier.

shuhaowu · 2019-08-27T14:19:32Z

test/integration/interrupt_resume_test.rb

+    #        contain this row.
+    #
+    #        Fixing this is somewhat non-trivial and likely require a more
+    #        extensive signal emitting system within Ghostferry.


I'm not quite sure how to solve this problem. Having this test is definitely better than not having this test. The race condition leading to test failure is also less likely than a successful test. Thus, I'm keeping this test here and added it to #107, where we track all the flaky tests.

How exactly are we handling this now?

Handling which part? The current signal emitting? There are some signal emitting from integrationferry tied to the event listeners on the DataIterator and the BinlogStreamer. There's no hook-in points in the InlineVerifier, BatchWriter, and etc.

hkdsun

Looks correct but some suggestions as usual 😄

Great tests 👍

hkdsun · 2019-08-29T12:23:47Z

inline_verifier.go

+				s.currentRowCount += uint64(count)
+			}
+		}
+	}


Rebase error? Can use func (s BinlogVerifySerializedStore) RowCount()

hkdsun · 2019-08-29T12:39:33Z

inline_verifier.go

+		}
+	}
+
+	return BinlogVerifySerializedStore(s.store)


Suggestion:

Change the type of the field s.store to BinlogVerifySerializedStore (feel free to remove the "Serialized" bit from the type name) so that we don't have to be going between named and unnamed type maps during (de)serialization?

Define a Copy() method on the BinlogVerifySerializedStore, as it's conventional in the standard library

Once you've done that, simplify this method to:

func (s *BinlogVerifyStore) Serialize() BinlogVerifySerializedStore { s.mutex.Lock() defer s.mutex.Unlock() return s.store.Copy() }

hkdsun · 2019-08-29T12:45:24Z

ferry.go

+
+	if f.inlineVerifier != nil {
+		serializedState.BinlogVerifyStore = f.inlineVerifier.reverifyStore.Serialize()
+	}


ahhh i don't like this after-the-fact mutation of serializedState. Everything was so nicely contained before

maybe you can pass it into the Serialize method, similar to how we build constructors?

hkdsun · 2019-08-29T13:38:30Z

test/integration/interrupt_resume_test.rb

+    batches_written = 0
+    ghostferry.on_status(Ghostferry::Status::AFTER_ROW_COPY) do
+      batches_written += 1
+      if batches_written >= 2


great 👍 curious how this is guaranteed though

After every row copy, integrationferry sends a HTTP request to the ruby integration test AFTER_ROW_COPY and the DataIterator is paused until the ruby code here returns and returns a 200 back to the integrationferry. Since we're only copying a single table, there's no concurrency.

Is this what you mean?

hkdsun · 2019-08-29T13:41:14Z

test/integration/interrupt_resume_test.rb

+    refute_nil dumped_state["BinlogVerifyStore"]["gftest"]["test_table_1"]
+
+    # FLAKY: AFTER_BINLOG_APPLY is emitted after the BinlogStreamer
+    #        finishes processing all of the event listeners. The block below


The block below

which block? 🤔

Meant to say the block above, but changed it to be a bit more specific.

hkdsun · 2019-08-29T13:48:57Z

test/integration/interrupt_resume_test.rb

+    chosen_id = result.first["MIN(id)"]
+
+    source_db.query("UPDATE #{DEFAULT_FULL_TABLE_NAME} SET data = 'data2' WHERE id = #{chosen_id}")
+    target_db.query("UPDATE #{DEFAULT_FULL_TABLE_NAME} SET data = 'corrupted' WHERE id = #{chosen_id}")


not sure what's the difference between this test and the previous one.. should the target db be updated here as well? the title makes it sound like only source should be corrupted/modified..

hmmm.. thinking more about... is it because we want the target to not accept the binlog copier's attempt to SET data = 'data2' 🤔

perhaps worth a code comment

hmmm.. thinking more about... is it because we want the target to not accept the binlog copier's attempt to SET data = 'data2' thinking

This is correct.

This test is to make sure that a binlog event that occured while Ghostferry has been interrupted is correctly picked up. Now, given that Ghostferry should correctly copy the row over upon resume, we'd have no way to observe that the InlineVerifier has also verified this row. Thus, if we set the data to corrupted here, the action of the InlineVerifier becomes observable.

Comment has been added.

fjordan · 2019-08-29T13:40:13Z

test/helpers/ghostferry_helper.rb

@@ -250,6 +252,17 @@ def start_ghostferry(resuming_state = nil)
                @logger.debug("stdout: #{line}")
              elsif reader == stderr
                @stderr << line
+                if line.start_with?("{")
+                  logline = JSON.parse(line)


maybe move this to a separate method? if json(line) log_json(line)

fjordan · 2019-08-29T14:16:24Z

inline_verifier.go

+	s := NewBinlogVerifyStore()
+
+	s.store = serialized
+	for db, _ := range s.store {


Can we use the RowCount() method above for this?

fjordan · 2019-08-29T14:22:45Z

ferry.go

@@ -708,7 +727,12 @@ func (f *Ferry) SerializeStateToJSON() (string, error) {
 		return "", err
 	}
 	serializedState := f.StateTracker.Serialize(f.Tables)
-	stateBytes, err := json.MarshalIndent(serializedState, "", "  ")
+
+	if f.inlineVerifier != nil {


Can we move this into f.StateTracker.Serialize?

fjordan · 2019-08-29T16:33:55Z

test/integration/interrupt_resume_test.rb

+    #        contain this row.
+    #
+    #        Fixing this is somewhat non-trivial and likely require a more
+    #        extensive signal emitting system within Ghostferry.


How exactly are we handling this now?

shuhaowu · 2019-09-03T04:58:51Z

I made some minor changes and added a unit test on BinlogVerifySerializedStore.RowCount() and BinlogVerifySerializedStore.Copy(). Do we need another quick review?

hkdsun

Thanks for addressing the comments 💯

shuhaowu force-pushed the inline-verifier-resumable branch 2 times, most recently from 10eddad to f778043 Compare August 26, 2019 22:44

shuhaowu added 2 commits August 27, 2019 10:09

Made inline verifier resumable

7d4de79

shuhaowu force-pushed the inline-verifier-resumable branch from f778043 to cab1a21 Compare August 27, 2019 14:09

shuhaowu commented Aug 27, 2019

View reviewed changes

shuhaowu requested review from fjordan and hkdsun August 27, 2019 14:28

hkdsun approved these changes Aug 29, 2019

View reviewed changes

fjordan reviewed Aug 29, 2019

View reviewed changes

Fixed feedback on PR

ac9e52f

hkdsun approved these changes Sep 3, 2019

View reviewed changes

fjordan approved these changes Sep 3, 2019

View reviewed changes

shuhaowu merged commit 79637d8 into Shopify:master Sep 3, 2019

shuhaowu mentioned this pull request Sep 9, 2019

Ghostferry interrupt/resume meta issue #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made InlineVerifier resumable #127

Made InlineVerifier resumable #127

shuhaowu commented Aug 26, 2019 •

edited

Loading

shuhaowu Aug 27, 2019

fjordan Aug 29, 2019

shuhaowu Aug 30, 2019

hkdsun left a comment

hkdsun Aug 29, 2019

hkdsun Aug 29, 2019

hkdsun Aug 29, 2019

hkdsun Aug 29, 2019

shuhaowu Aug 30, 2019

hkdsun Aug 29, 2019

shuhaowu Aug 30, 2019

hkdsun Aug 29, 2019

shuhaowu Sep 3, 2019

fjordan Aug 29, 2019

fjordan Aug 29, 2019

fjordan Aug 29, 2019

fjordan Aug 29, 2019

shuhaowu commented Sep 3, 2019

hkdsun left a comment

Made InlineVerifier resumable #127

Made InlineVerifier resumable #127

Conversation

shuhaowu commented Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkdsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhaowu commented Sep 3, 2019

hkdsun left a comment

Choose a reason for hiding this comment

shuhaowu commented Aug 26, 2019 •

edited

Loading