insom
commented
2 years ago I'm not sure if I understand the implications of the dml_event changes with MySQL 5.7 (and the existing code dealing with JSON in dml_event as pointed out in my comment above), which remains to be our most important use case for now. Since this is a change to the "core" part of Ghostferry, can you perhaps give me some further elaboration so I feel a bit safer with this code?
We've added a few more comments but basically nothing fundamental changed between MySQL 5.7 and 8.0 that affects Ghostferry itself. The changes that we made around string vs []byte for JSON types is only related to pulling in a newer version of this library -- even without MySQL 8.0 support revendoring this library would have brought these changes.
And, as you pointed out, the rest of the changes are actually only in the tests -- assumptions that are made in the internals of MySQL that don't carry through to Ghostferry itself, just the test suite (i.e. column counts and reserved words).
Apart from that, I left a few comments on places where the code is surprising, but is not documented inline with comments. Ghostferry is a fairly tricky code to get right, and I tried my best to annotated "surprising" code with comments to make them less surprising and give the next person working on it more context to fix/upgrade the code. This made my own job a lot easier and it would be preferable to continue that tradition.
Hopefully the latest batch of comments covers this, especially around the reason for skipped tests and for casting strings back into []byte despite the API change in the vendored library.
Co-authored-by: Aaron Brady aaron.brady@shopify.com