kasparjarek commented on PR #31:
URL: https://github.com/apache/pulsar-connectors/pull/31#issuecomment-4906555348
@david-streamlio
**Re 1: Testing**
Thanks for providing the tests. I am ok with the current test state.
However, these tests verify the converters work in isolation but don't test the
actual production code path - that `AbstractKafkaConnectSource.open()` uses
these JSON converters for offset storage. Testing the production path requires
the broker test harness, which is blocked by the missing pulsar-broker
artifacts. Once the build is fixed, I would prefer to add a proper E2E
integration test, so I have added TODO comment into the test file. This would
test the actual bug scenario and could potentially replace the current isolated
tests. Are you ok with it?
**Re 3: Silent offset reset documentation**
I agree upgrade notes (option A) are needed here. I'm happy to prepare an
upgrade note. Where should it go? I don't see a CHANGELOG file in this repo,
and from the git history it looks like release notes are primarily captured in
PR descriptions. Should I add an "Upgrade Notes" section to this PR's
description, or is there another location for this?
Regarding the WARN log (option B) I explored it but found it's more
problematic. A non-empty offset topic doesn't necessarily mean this specific
connector had a previous offset - multiple connectors can share a single offset
topic (which is common, e.g., Debezium connectors use a default shared topic
name). This would cause false positives whenever a new connector starts against
an existing shared topic. The detection logic also adds code complexity that
isn't worth the limited value it provides in my opinion.
**Re 4: Decision record + backport**
Happy to help document the decision on issue #30 or backport this fix if
needed. Just tell me where/how you want it. Regarding backporting the fix, I
would lean toward the backward-compatible config-based approach to avoid the
silent offset reset issue discussed in point 3 - we shouldn't break existing
working setups in a minor release. But of course it up to maintainers to decide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
