flink icon indicating copy to clipboard operation
flink copied to clipboard

Published 20 hours ago • verified publisher icon apache

[FLINK-25916][connector-kafka] Using upsert-kafka with a flush buffer…

Open paul8263 opened this issue 3 years ago • 3 comments

… results in Null Pointer Exception

What is the purpose of the change

Allow flink-connectors/flink-connector-kafka/src/main/java/org/apache/flink/streaming/connectors/kafka/table/ReducingUpsertWriter.WrappedContext to set timestamp with NULL value in order to avoid NPE.

Brief change log

  • flink-connectors/flink-connector-kafka/src/main/java/org/apache/flink/streaming/connectors/kafka/table/ReducingUpsertWriter.java
  • flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/ReducingUpsertWriterTest.java

Verifying this change

This change is already covered by existing tests, such as flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/ReducingUpsertWriterTest::testWriteDataWithNullTimestamp().

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no

paul8263 avatar Jul 22 '22 08:07 paul8263

CI report:

  • 5a66165ebc9bc2824afcd3ab13b0fd0b62bd66e3 Azure: FAILURE
Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

flinkbot avatar Jul 22 '22 08:07 flinkbot

Hi, any plans on merging this? :smile:

jaumebecks avatar Aug 25 '22 13:08 jaumebecks

@paul8263 I'm currently facing this issue, do you know when it will be merge it? thanks

mhv666 avatar Sep 14 '22 12:09 mhv666

@PatrickRen WDYT?

MartijnVisser avatar Oct 03 '22 17:10 MartijnVisser

@mas-chen Can and would you like to review this, since you have expressed some interest in the Kafka connector?

MartijnVisser avatar Oct 05 '22 19:10 MartijnVisser

@MartijnVisser it seems @paul8263 is not responding to the requested changes, we may create a similar PR in the following days for fixing this, does that sound good to you?

jaumebecks avatar Dec 14 '22 14:12 jaumebecks

@jaumebecks Yes, but let's not create that right now. We're in the process of externalizing Kafka (see https://github.com/apache/flink-connector-kafka/pull/1) and when that's done, all PRs should be created in that repo, not in the Flink main repo

MartijnVisser avatar Dec 14 '22 14:12 MartijnVisser

@MartijnVisser it seems @paul8263 is not responding to the requested changes, we may create a similar PR in the following days for fixing this, does that sound good to you?

Hi @jaumebecks , Sorry for the late response. I will do it these days.

paul8263 avatar Jan 03 '23 01:01 paul8263

Hi @MartijnVisser and @jaumebecks ,

I updated the unit test and I am currently waiting for the CI result.

If there are other issues that need any changes, please let me know. Thank you very much.

paul8263 avatar Jan 03 '23 03:01 paul8263

Hi @MartijnVisser, now that https://github.com/apache/flink-connector-kafka/pull/1 is merged, what should we do with this PR, shall we move it there, or is it okay to maintain it? Thanks

jaumebecks avatar Feb 07 '23 14:02 jaumebecks

@jaumebecks It should be moved; Flink 1.17 will be the latest release that ships with a Kafka Connector, all new features should go into the externalized repo

MartijnVisser avatar Feb 07 '23 14:02 MartijnVisser

Thanks @MartijnVisser! @paul8263 wdyt? Will you take over this migration into the new repo, otherwise we can try it

jaumebecks avatar Feb 07 '23 14:02 jaumebecks

Hi @jaumebecks, Thanks for reviewing the PR. I'd like to do the migration.

paul8263 avatar Feb 10 '23 06:02 paul8263

Shall we close this PR and move the conversation to https://github.com/apache/flink-connector-kafka/pull/5?

jaumebecks avatar Mar 27 '23 08:03 jaumebecks

Shall we close this PR and move the conversation to https://github.com/apache/flink-connector-kafka/pull/5?

yes please. We're removing the Kafka connector code from apache/flink:main now.

tzulitai avatar Mar 27 '23 15:03 tzulitai

Shall we close this PR and move the conversation to apache/flink-connector-kafka#5?

yes please. We're removing the Kafka connector code from apache/flink:main now.

Can anyone please close this PR? I see you still push commits @paul8263, can you please move them to apache/flink-connector-kafka#5?

jaumebecks avatar Mar 28 '23 06:03 jaumebecks

@flinkbot run azure

paul8263 avatar Mar 29 '23 05:03 paul8263

@paul8263 The Flink Kafka connector resides in it's own repository, if this code change is still relevant, please open the PR in https://github.com/apache/flink-connector-kafka

MartijnVisser avatar Oct 11 '23 18:10 MartijnVisser