SEPURI-SAI-KRISHNA commented on PR #12927:
URL: https://github.com/apache/seatunnel/pull/12927#issuecomment-6310163203
Thank you, this was a sharp review, and issue 1 is a real defect in my test
rather than a CI artefact.
**Issue 1, taken.** Confirmed from the fork run log before changing
anything: `row num :200`, `:300`, `:400`, `:500` and `:600` against `MAX_ROW
ruleValue=100.0`, with the first invocation passing at exactly 100. Your cause
is right: `KafkaIT` is a `@TestTemplate` run once per engine container against
one shared broker, so a fixed topic gains another 100 records per run.
Fixed with the pattern the restore and start-mode tests in this class
already use: `startModeTopic(container, "avro-tombstone")`,
`createKafkaTopic(topic)` so it is registered in `dynamicTopics` and dropped in
`tearDown`, and the name passed to the job as `${topic}` via `executeJob(conf,
Collections.singletonList("topic=" + topic))`.
**Issue 2, taken as the doc narrowing.** You are right about
`ProtobufDeserializationSchema` on current `dev`:
`DynamicMessage.parseFrom(descriptor, message)` has no null check. The docs now
name the formats that do guard and say `protobuf` does not. I left
`compatible_kafka_connect_json` out: it guards `msg == null`, the record rather
than a null value, but it never goes through `deserialize(byte[])` and I did
not run it, so I did not want to assert an outcome. Rather than widen this PR
into the protobuf schemas I will open that separately.
**Issue 3, keeping the empty-value branch**, for consistency with text,
canal, debezium and ogg, which all guard `message == null || message.length ==
0`. You are right that "every other format" was overstated: I checked each one,
and `json` and `maxwell_json` guard null only. The docs now say so instead of
generalising.
Checking that list caught two mistakes of my own: my first draft listed
`csv`, which is not a Kafka `MessageFormat` value at all, and included
`compatible_debezium_json`, whose `deserialize(byte[])` just throws
`UnsupportedEncodingException` as a CDC-only path. Both are gone.
**Issue 4, you are right and I was wrong.** From the job log, the failure on
this head is
`LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled`,
`InvalidClassException` on the `AbstractMultiCatalogTransform`
serialVersionUID, which arrived with #12194 and is tracked in #12929. The
reactor stops at `Transforms V2`, so `connector-jdbc` is never reached and
#12874 is not the cause here. That also means `connector-kafka` unit tests were
skipped, as you say. Description corrected.
--
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]
