You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "johnjcasey (via GitHub)" <gi...@apache.org> on 2023/11/28 16:39:55 UTC
[PR] Feature/kafka read dlq [beam]
johnjcasey opened a new pull request, #29546:
URL: https://github.com/apache/beam/pull/29546
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey merged PR #29546:
URL: https://github.com/apache/beam/pull/29546
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #29546:
URL: https://github.com/apache/beam/pull/29546#discussion_r1416545631
##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##########
@@ -739,6 +754,9 @@ abstract Builder<K, V> setValueDeserializerProvider(
abstract Builder<K, V> setCheckStopReadingFn(@Nullable CheckStopReadingFn checkStopReadingFn);
+ abstract Builder<K, V> setBadRecordErrorHandler(
+ @Nullable ErrorHandler<BadRecord, ?> badRecordErrorHandler);
Review Comment:
When will we ever put null in this parameter?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29546:
URL: https://github.com/apache/beam/pull/29546#issuecomment-1841799360
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #29546:
URL: https://github.com/apache/beam/pull/29546#discussion_r1416541164
##########
sdks/java/io/kafka/kafka-01103/build.gradle:
##########
@@ -18,6 +18,7 @@
project.ext {
delimited="0.11.0.3"
undelimited="01103"
+ sdfCompatable=false
Review Comment:
is "Compatable" a typo here?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey commented on code in PR #29546:
URL: https://github.com/apache/beam/pull/29546#discussion_r1417452924
##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/ReadFromKafkaDoFn.java:
##########
@@ -431,36 +449,52 @@ public ProcessContinuation processElement(
if (!tracker.tryClaim(rawRecord.offset())) {
return ProcessContinuation.stop();
}
- KafkaRecord<K, V> kafkaRecord =
- new KafkaRecord<>(
- rawRecord.topic(),
- rawRecord.partition(),
- rawRecord.offset(),
- ConsumerSpEL.getRecordTimestamp(rawRecord),
- ConsumerSpEL.getRecordTimestampType(rawRecord),
- ConsumerSpEL.hasHeaders() ? rawRecord.headers() : null,
- ConsumerSpEL.deserializeKey(keyDeserializerInstance, rawRecord),
- ConsumerSpEL.deserializeValue(valueDeserializerInstance, rawRecord));
- int recordSize =
- (rawRecord.key() == null ? 0 : rawRecord.key().length)
- + (rawRecord.value() == null ? 0 : rawRecord.value().length);
- avgRecordSize
- .getUnchecked(kafkaSourceDescriptor.getTopicPartition())
- .update(recordSize, rawRecord.offset() - expectedOffset);
- rawSizes.update(recordSize);
- expectedOffset = rawRecord.offset() + 1;
- Instant outputTimestamp;
- // The outputTimestamp and watermark will be computed by timestampPolicy, where the
- // WatermarkEstimator should be a manual one.
- if (timestampPolicy != null) {
- TimestampPolicyContext context =
- updateWatermarkManually(timestampPolicy, watermarkEstimator, tracker);
- outputTimestamp = timestampPolicy.getTimestampForRecord(context, kafkaRecord);
- } else {
- Preconditions.checkStateNotNull(this.extractOutputTimestampFn);
- outputTimestamp = extractOutputTimestampFn.apply(kafkaRecord);
+ try {
+ KafkaRecord<K, V> kafkaRecord =
+ new KafkaRecord<>(
+ rawRecord.topic(),
+ rawRecord.partition(),
+ rawRecord.offset(),
+ ConsumerSpEL.getRecordTimestamp(rawRecord),
+ ConsumerSpEL.getRecordTimestampType(rawRecord),
+ ConsumerSpEL.hasHeaders() ? rawRecord.headers() : null,
+ ConsumerSpEL.deserializeKey(keyDeserializerInstance, rawRecord),
+ ConsumerSpEL.deserializeValue(valueDeserializerInstance, rawRecord));
+ int recordSize =
+ (rawRecord.key() == null ? 0 : rawRecord.key().length)
+ + (rawRecord.value() == null ? 0 : rawRecord.value().length);
+ avgRecordSize
+ .getUnchecked(kafkaSourceDescriptor.getTopicPartition())
+ .update(recordSize, rawRecord.offset() - expectedOffset);
+ rawSizes.update(recordSize);
+ expectedOffset = rawRecord.offset() + 1;
+ Instant outputTimestamp;
+ // The outputTimestamp and watermark will be computed by timestampPolicy, where the
+ // WatermarkEstimator should be a manual one.
+ if (timestampPolicy != null) {
+ TimestampPolicyContext context =
+ updateWatermarkManually(timestampPolicy, watermarkEstimator, tracker);
+ outputTimestamp = timestampPolicy.getTimestampForRecord(context, kafkaRecord);
+ } else {
+ Preconditions.checkStateNotNull(this.extractOutputTimestampFn);
+ outputTimestamp = extractOutputTimestampFn.apply(kafkaRecord);
+ }
+ receiver
+ .get(recordTag)
+ .outputWithTimestamp(KV.of(kafkaSourceDescriptor, kafkaRecord), outputTimestamp);
+ } catch (SerializationException e) {
+ // This exception should only occur during the key and value deserialization when
+ // creating the Kafka Record
+ badRecordRouter.route(
Review Comment:
We do not
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey commented on PR #29546:
URL: https://github.com/apache/beam/pull/29546#issuecomment-1843302521
GCP IO Direct test is a flake, whitespace failure is due to files this PR doesn't touch
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #29546:
URL: https://github.com/apache/beam/pull/29546#discussion_r1416553565
##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/ReadFromKafkaDoFn.java:
##########
@@ -431,36 +449,52 @@ public ProcessContinuation processElement(
if (!tracker.tryClaim(rawRecord.offset())) {
return ProcessContinuation.stop();
}
- KafkaRecord<K, V> kafkaRecord =
- new KafkaRecord<>(
- rawRecord.topic(),
- rawRecord.partition(),
- rawRecord.offset(),
- ConsumerSpEL.getRecordTimestamp(rawRecord),
- ConsumerSpEL.getRecordTimestampType(rawRecord),
- ConsumerSpEL.hasHeaders() ? rawRecord.headers() : null,
- ConsumerSpEL.deserializeKey(keyDeserializerInstance, rawRecord),
- ConsumerSpEL.deserializeValue(valueDeserializerInstance, rawRecord));
- int recordSize =
- (rawRecord.key() == null ? 0 : rawRecord.key().length)
- + (rawRecord.value() == null ? 0 : rawRecord.value().length);
- avgRecordSize
- .getUnchecked(kafkaSourceDescriptor.getTopicPartition())
- .update(recordSize, rawRecord.offset() - expectedOffset);
- rawSizes.update(recordSize);
- expectedOffset = rawRecord.offset() + 1;
- Instant outputTimestamp;
- // The outputTimestamp and watermark will be computed by timestampPolicy, where the
- // WatermarkEstimator should be a manual one.
- if (timestampPolicy != null) {
- TimestampPolicyContext context =
- updateWatermarkManually(timestampPolicy, watermarkEstimator, tracker);
- outputTimestamp = timestampPolicy.getTimestampForRecord(context, kafkaRecord);
- } else {
- Preconditions.checkStateNotNull(this.extractOutputTimestampFn);
- outputTimestamp = extractOutputTimestampFn.apply(kafkaRecord);
+ try {
+ KafkaRecord<K, V> kafkaRecord =
+ new KafkaRecord<>(
+ rawRecord.topic(),
+ rawRecord.partition(),
+ rawRecord.offset(),
+ ConsumerSpEL.getRecordTimestamp(rawRecord),
+ ConsumerSpEL.getRecordTimestampType(rawRecord),
+ ConsumerSpEL.hasHeaders() ? rawRecord.headers() : null,
+ ConsumerSpEL.deserializeKey(keyDeserializerInstance, rawRecord),
+ ConsumerSpEL.deserializeValue(valueDeserializerInstance, rawRecord));
+ int recordSize =
+ (rawRecord.key() == null ? 0 : rawRecord.key().length)
+ + (rawRecord.value() == null ? 0 : rawRecord.value().length);
+ avgRecordSize
+ .getUnchecked(kafkaSourceDescriptor.getTopicPartition())
+ .update(recordSize, rawRecord.offset() - expectedOffset);
+ rawSizes.update(recordSize);
+ expectedOffset = rawRecord.offset() + 1;
+ Instant outputTimestamp;
+ // The outputTimestamp and watermark will be computed by timestampPolicy, where the
+ // WatermarkEstimator should be a manual one.
+ if (timestampPolicy != null) {
+ TimestampPolicyContext context =
+ updateWatermarkManually(timestampPolicy, watermarkEstimator, tracker);
+ outputTimestamp = timestampPolicy.getTimestampForRecord(context, kafkaRecord);
+ } else {
+ Preconditions.checkStateNotNull(this.extractOutputTimestampFn);
+ outputTimestamp = extractOutputTimestampFn.apply(kafkaRecord);
+ }
+ receiver
+ .get(recordTag)
+ .outputWithTimestamp(KV.of(kafkaSourceDescriptor, kafkaRecord), outputTimestamp);
+ } catch (SerializationException e) {
+ // This exception should only occur during the key and value deserialization when
+ // creating the Kafka Record
+ badRecordRouter.route(
Review Comment:
QQ: do we have timestamp in DLQ?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Error Handling to Kafka IO [beam]
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey commented on code in PR #29546:
URL: https://github.com/apache/beam/pull/29546#discussion_r1417451895
##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##########
@@ -739,6 +754,9 @@ abstract Builder<K, V> setValueDeserializerProvider(
abstract Builder<K, V> setCheckStopReadingFn(@Nullable CheckStopReadingFn checkStopReadingFn);
+ abstract Builder<K, V> setBadRecordErrorHandler(
+ @Nullable ErrorHandler<BadRecord, ?> badRecordErrorHandler);
Review Comment:
We will never set it to be null. This is @Nullable to indicate that the property may not be set, and that is valid
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org