debezium/dbz#1518 fix EventDeserializer composition#19
Conversation
|
@adelajaworowska Unfortunately as you are aware the CI is now broken so I'd be glad if you could run the tests locally and let me know if they passes when the PR is ready for review. Thanks for understanding. |
| new MariadbGtidListEventDataDeserializer()); | ||
| eventDataDeserializers.put(EventType.TRANSACTION_PAYLOAD, | ||
| new TransactionPayloadEventDataDeserializer()); | ||
| new TransactionPayloadEventDataDeserializer(this)); |
There was a problem hiding this comment.
What about other deserializers? Does only TransactionPayloadEventDataDeserializer needs to have this parameters?
There was a problem hiding this comment.
I went through all other deserializers' code and TransactionPayloadEventDataDeserializer is the only one that needs a fix
| public static final int OTW_PAYLOAD_COMPRESSION_TYPE_FIELD = 2; | ||
| public static final int OTW_PAYLOAD_UNCOMPRESSED_SIZE_FIELD = 3; | ||
|
|
||
| private final EventDeserializer parentEventDeserializer; |
There was a problem hiding this comment.
Will this cause cyclic reference?
There was a problem hiding this comment.
You're right. I changed the approach.
adelajaworowska
force-pushed
the
eventDeserializer-composition-fix
branch
from
9c922e6 to
99a612e
Compare
| if (eventDataDeserializer instanceof TransactionPayloadEventDataDeserializer) { | ||
| TransactionPayloadEventDataDeserializer deserializer = | ||
| (TransactionPayloadEventDataDeserializer) eventDataDeserializer; | ||
| if (!compatibilitySet.isEmpty()) { |
There was a problem hiding this comment.
So this works for newly created EventDeserializer when you first set compatibility mode. But then if someone's case is to remove compatibility mode it will stay inside TransactionPayloadEventDataDeserializer. Doesn't it? I assume always compatibilitySet should propagate relevant to TransactionPayloadEventDataDeserializer no matter if it is set or unset.
There was a problem hiding this comment.
Currently the library gives us no way to remove compatibilityMode. The only method we can call is setCompatibilityMode() that enforce at least one argument.
But in case it is changed in the future, I modified this code so we just take compatibilitySet as it is and pass it to TransactionPayloadEventDataDeserializer. WDYT now?
adelajaworowska
force-pushed
the
eventDeserializer-composition-fix
branch
2 times, most recently
from
9657ad3 to
17553e6
Compare
Signed-off-by: Adela Jaworowska-Nowak <adela.jaworowska@gmail.com>
Signed-off-by: Adela Jaworowska-Nowak <adela.jaworowska@gmail.com>
adelajaworowska
force-pushed
the
eventDeserializer-composition-fix
branch
from
4d5e649 to
50f3484
Compare
sfc-gh-pkochmanski
left a comment
sfc-gh-pkochmanski
left a comment
There was a problem hiding this comment.
I left one comment, but overall, good job!
| ArrayList<Event> decompressedEvents = new ArrayList<>(); | ||
| EventDeserializer transactionPayloadEventDeserializer = new EventDeserializer(); | ||
| if (!compatibilitySet.isEmpty()) { | ||
| EventDeserializer.CompatibilityMode[] compatibilityModes = compatibilitySet.toArray(new EventDeserializer.CompatibilityMode[0]); |
There was a problem hiding this comment.
I would create a similar function, like the one in this file: public void setCompatibilityModes(EnumSet<EventDeserializer.CompatibilityMode> compatibilityModes) in EventDeserializer, to pass the compatibility mode without unpacking it into an array for every transaction payload event.
Signed-off-by: Adela Jaworowska-Nowak <adela.jaworowska@gmail.com>
|
LGTM. Thank you for the change. IMHO, it's ready for further review from the maintainers. Thanks you in advance for taking a look at @adelajaworowska's PR. |
|
@jpechane the PR is ready for review. Tests pass locally besides TransactionPayloadIntegrationTest that fails also on main branch. For this one I created a PR with the fix. |
|
@adelajaworowska Applied, thanks! |
4 participants
Fixes debezium/dbz#1518
When compatibility mode is set TransactionPayloadEventDataDeserializer() should be aware about it and in deserialize method when it creates new deserializer
EventDeserializer transactionPayloadEventDeserializer = new EventDeserializer(); it should set compatibility mode from top level object.FYI, the issue was also raised in osheroff repo: osheroff#127, osheroff#138 but never addressed.