boring-cyborg[bot] commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034157470
Thanks for opening this pull request! Please check out our contributing
guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)
--
This is a
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034169137
@MartijnVisser Thanks for the quick response. There are existing tests
covering this class and exactly this line of code and I also verified it pass.
I'm happy to add more tests
MartijnVisser commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034536889
> There are existing tests covering this class and exactly this line of code
and I also verified it pass. I'm happy to add more tests to ensure the user
code class loader
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034668045
@MartijnVisser True. I added a couple of tests that would fail without my
change. Hope it looks good now. Please let me know if anything else is needed.
--
This is an automate
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034716402
@MartijnVisser May I have your advice on the use of
`TemporaryClassLoaderContext` in the implementation? It looks unnecessary to
me. Do you think I should just remove it in thi
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2034714493
@MartijnVisser May I have your advice on the use of
`TemporaryClassLoaderContext` in the implementation? It looks unnecessary to
me. Do you think I should just remove it in thi
MartijnVisser commented on code in PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#discussion_r1549969179
##
flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapperTest.java:
##
@@ -0,0 +1,28 @@
+package org.apache.f
hugogu commented on code in PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#discussion_r1550913873
##
flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapperTest.java:
##
@@ -0,0 +1,28 @@
+package org.apache.flink.co
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2039767446
@MartijnVisser I also fix a few style errors, please let me know if it is
good now or any other change to make. Thanks for your review.
--
This is an automated message from t
morazow commented on code in PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#discussion_r1555308164
##
flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapperTest.java:
##
@@ -0,0 +1,63 @@
+package org.apache.flink.c
morazow commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2041992850
Thanks @hugogu for the PR! I have added some comments, please have a look.
Do you know why the CI is failing?
--
This is an automated message from the Apache Git Servic
hugogu commented on code in PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#discussion_r1555387996
##
flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapperTest.java:
##
@@ -0,0 +1,63 @@
+package org.apache.flink.co
hugogu commented on code in PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#discussion_r1555389033
##
flink-connector-kafka/src/main/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapper.java:
##
@@ -61,17 +61,12 @@ class KafkaSerializerWrapper
morazow commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2044272245
Looks good, thanks @hugogu for quick update! Hopefully the CI is green this
time 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, pl
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2049754379
@morazow Thanks for your approval. I have just rebased this PR to include
the build fix made in main branch. Hopefully the build would success this time.
I also noticed th
hugogu commented on PR #89:
URL:
https://github.com/apache/flink-connector-kafka/pull/89#issuecomment-2065806444
Hi @uce and @rmetzger, do you mind reviewing the change and let me know if
any change is required before merging. Thanks.
--
This is an automated message from the Apache Git
16 matches
Mail list logo
