Detect ddl add handlers

[shivnagarajan]

This is a slightly alternate version of

#322

@pawandubey

[shivnagarajan]

This changes the code to provide the ability to add a binlogEvent handler for individual events. The golang test has been updated to test adding such a handler for a query event and validates that it actually works.

Since the code no longer needs to parse the binlog events explicitly, we did not need to pull in the full parser for the ddl events that we had earlier.

[shuhaowu]

So the defaultEventHandler fires events to the eventListeners, but if one were to override the eventHandler, eventListeners will no longer be called? This also means that the default event listener that handles the row event (by sending it to the BinlogWriter) will no longer be called. You would have to manually reregister handleRowEvent as a eventHandler? This feels very confusing and somewhat error prone? Are there any cases where we don't want to row events to be handled as well?

[pawandubey]

@shivnagarajan and I discussed this briefly: the event listeners would only be missed if a custom handler for ROWS_EVENT was registered (overwriting the default handler behaviour). The problem is that the callers would not have access to s.eventListeners if they did want to fire them off themselves. That's probably okay since we are providing a hook into event handling and not a hook into the bodies of built-in event handlers themselves.

There are a few ways to tackle this, that I wanted to summarize:

• Accept this limitation and note it in the documentation (possibly as a gotcha).
  This is the current approach. If there is demand for this, we can later come back and adopt one of the alternative approaches later.

• Always firing off the eventListeners for ROWS_EVENT, even if a custom handler had been registered.
  This would solve the specific problem
  This solves the specific problem of firing off the listeners for ROWS_EVENT without increasing the surface area of the API exposed to the handler, but it will be a surprising, snowflake-y behaviour which cannot be turned off even if you wanted to.

• Make eventListeners available to the callers inside the handler so that they can do with them as they wish.
  This solves the whole problem at the cost of muddling the handler signature with data unrelated to the event state. Moreover, the listeners are currently only be useful for one specific event but would need to be a part of the signature for handlers of all events (this is perhaps not too bad but still seems unnecessary).

• Another approach not considered here?

I think we should go with the first option for now and revisit later if needed, FWIW.

---

Separately, I think if we wanted to be "truly" flexible in handler design then we should get to a place where all our default handlers could be registered as custom handlers instead of being implemented in a private fallback function. But we don't need to take on that complexity all at once.

[shuhaowu]

OK the first option sounds OK for the time being.

I suspect this part of the code will need to be revisited if we ever look into a larger restructure of Ghostferry as I think the complexity is increasing with this change (and also would with any other changes proposed). Perhaps the callback system would need to be revamped 🤷 , but that's a problem for another day.

[shivnagarajan]

I just rebased this to latest master. Will clean up a little based on the remaining comments.

[pawandubey]

Noice!

[pawandubey]

@shivnagarajan and I discussed this briefly: the event listeners would only be missed if a custom handler for ROWS_EVENT was registered (overwriting the default handler behaviour). The problem is that the callers would not have access to s.eventListeners if they did want to fire them off themselves. That's probably okay since we are providing a hook into event handling and not a hook into the bodies of built-in event handlers themselves.

There are a few ways to tackle this, that I wanted to summarize:

• Accept this limitation and note it in the documentation (possibly as a gotcha).
  This is the current approach. If there is demand for this, we can later come back and adopt one of the alternative approaches later.

• Always firing off the eventListeners for ROWS_EVENT, even if a custom handler had been registered.
  This would solve the specific problem
  This solves the specific problem of firing off the listeners for ROWS_EVENT without increasing the surface area of the API exposed to the handler, but it will be a surprising, snowflake-y behaviour which cannot be turned off even if you wanted to.

• Make eventListeners available to the callers inside the handler so that they can do with them as they wish.
  This solves the whole problem at the cost of muddling the handler signature with data unrelated to the event state. Moreover, the listeners are currently only be useful for one specific event but would need to be a part of the signature for handlers of all events (this is perhaps not too bad but still seems unnecessary).

• Another approach not considered here?

I think we should go with the first option for now and revisit later if needed, FWIW.

---

Separately, I think if we wanted to be "truly" flexible in handler design then we should get to a place where all our default handlers could be registered as custom handlers instead of being implemented in a private fallback function. But we don't need to take on that complexity all at once.

[shuhaowu]

I requested a few minor comments, as I feel the complexity of the code in the BinlogStreamer increasing a bit, but that's likely OK for now.