DanielLeens commented on PR #12048: URL: https://github.com/apache/seatunnel/pull/12048#issuecomment-5662001065
Thanks, @SEZ9 — confirming the no-op assessment on `f3db7dfd3de2` matches what I independently verified as well (only shared registration files touched, all for unrelated connectors landing on `dev`). On F1 and F3, both were already answered in detail in my first-round review (https://github.com/apache/seatunnel/pull/12048#pullrequestreview-5088543092), unchanged since: - **F1 (retention-trimmed checkpointed sequence number):** this is intentionally fail-fast, not silently swallowed. `AzureEventHubsSourceSplitReader.validateAndGetNextSequenceNumber()` throws `AzureEventHubsConnectorException` the moment a received event's sequence number doesn't match the expected next value — e.g. if Event Hubs retention has purged the checkpointed position — rather than silently reseeking to the partition's current start/end and quietly skipping data. That's covered by the `sequenceGapFailsWithoutSilentlySkippingCheckpointPosition` test and matches the PR's documented contract. I treat this as a deliberate, correct design choice, same category as F1/F6 on the LocalFile PR — not a defect. - **F3 (connection-string masking on every output path):** also verified directly, not just at the `ConfigShadeUtils` entry point. `"connection_string"` is in `DEFAULT_SENSITIVE_KEYWORDS` (exact-key masking, locked in by `ConfigBuilderTest#testConfigDesensitizationMasksConnectionStrings`); every Azure SDK call site (`partitionIds()`, `initialSequenceNumber()`, `receive()`, `close()`, constructor) wraps `RuntimeException` into a distinct `AzureEventHubsConnectorException`, and I checked every `throw`/`log.info`/`log.error` call site in the diff — none interpolate the raw connection string into a message, only `eventHubName`/`partitionId`/sequence numbers. The EntityPath-rejection path is validated at config-build time (`AzureEventHubsSourceConfig.from`) and covered by `AzureEventHubsSourceConfigTest`. I re-confirmed this was untouched again in my `9377e822c` round. Consider F3 closed. **F2 (`ServicesResourceTransformer` for the Azure SDK's `ServiceLoader` providers) — this one I have to be honest about: I hadn't specifically checked it before, and it's a legitimate gap in my prior rounds, not something to wave off.** I went and read the current shade config (`connector-azure-event-hubs/pom.xml`) directly: there is no `ServicesResourceTransformer` configured, only package `<relocation>`s for `com.azure`, `com.fasterxml.jackson`, `io.netty`, `org.apache.qpid.proton`, `com.microsoft.azure`, `org.reactivestreams`, `reactor`. Mechanically, that matters because plain package relocation rewrites class bytecode references but does not rewrite `META-INF/services/<interface-FQCN>` file names or the implementation-class names inside them — so after relocation, a `ServiceLoader.load(SomeAzureInterface.class)` call using the *relocated* class can fail to find the (still original-named) service file, and provider discovery silently comes back empty. That said, before treating this as a live defect I checked precedent rather than assuming: `connector-azure-queue-storage`, an already-merged, long-shipping connector, relocates the exact same `com.azure`/jackson/reactor/netty packages with the identical `maven-shade-plugin` pattern and also has **no** `ServicesResourceTransformer`. So either this is a latent issue affecting both connectors equally (pre-existing, not introduced by this PR), or it doesn't actually bite in practice for the Azure SDK surfaces these two connectors exercise (e.g. if the relevant client build paths don't hard-depend on `ServiceLoader`-discovered providers, or fall back safely when none are found). I have not been able to confirm which from static reading alone, and I don't want to assert a false certainty either way. @goutamadwant — the author summary mentioned the Event Hubs emulator E2E passing; could you confirm whether that run exercises the packaged/shaded jar (i.e. the built `connector-azure-event-hubs` artifact via the dist/e2e module against the real emulator container), rather than test-scope classes compiled against the unshaded dependency tree? If it's genuinely running the shaded jar against a live emulator and the AMQP/HTTP transport comes up cleanly, that's real evidence F2 isn't biting at runtime — more convincing than the sibling-connector precedent alone. If it's *not* exercising the shaded jar, this is worth a closer look before merge, even though it wouldn't be a regression relative to the existing `connector-azure-queue-storage` pattern. -- 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]
