david-streamlio commented on PR #31: URL: https://github.com/apache/pulsar-connectors/pull/31#issuecomment-4916014377
@kasparjarek Thanks for the thorough follow-up — responses inline: **Re 1: Testing** Yes, we're aligned on adding the E2E integration test once the broker artifacts are published, and the TODO you added captures the scenario well. One clarification on "could potentially replace the current isolated tests": I'd keep both layers rather than replacing. They serve different purposes — the unit tests are fast, run in CI today, and pin the regression at the converter level (if someone swaps the offset converters back to schema-aware ones, they fail immediately with a clear signal); the E2E test proves `open()` actually wires those converters into the production path. This module's own history is the cautionary tale here: broker-harness tests are exactly the kind that get disabled when artifacts drift, and if the E2E test had *replaced* the unit tests, we'd be back to zero coverage on this fix the next time that happens. Unit + integration as complementary layers, not either/or. **Re 3: Upgrade notes** An "Upgrade Notes" section in this PR's description works — that matches how release notes are captured in this repo today. @lhotari, worth confirming whether this should also land as a note in the pulsar-site docs, since the silent-offset-reset behavior on upgrade is the kind of thing users will search for after the fact. Agreed on dropping the WARN log (option B) — the shared-offset-topic false positive is a solid objection I hadn't weighed; a warning that fires for every fresh connector on a shared Debezium-style topic is worse than no warning. **Re 4: Decision record + backport** Your instinct matches mine — config-based backward-compatible variant for the 4.x branches, hardcoded JSON here for the major. But that's a maintainer call. @lhotari, could you weigh in on (a) recording the hardcoded-JSON decision on #30 and (b) whether branch-4.2 and earlier should get the config-based variant, this fix as-is, or nothing? **On CI:** the red check here isn't this PR — every run in this repo currently fails at the Setup Gradle step because the pinned `gradle/actions/setup-gradle` SHA was dropped from the ASF actions allowlist (same breakage as apache/pulsar#26156). Fix is up in #37; once that merges, this branch needs a base update to get a real run of the newly re-enabled tests. -- 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]
