ferenc-csaky commented on code in PR #127:
URL:
https://github.com/apache/flink-connector-pulsar/pull/127#discussion_r3879890890
##########
flink-connector-pulsar-e2e-tests/src/test/resources/log4j2-test.properties:
##########
@@ -23,6 +23,6 @@ rootLogger.appenderRef.test.ref = TestLogger
appender.testlogger.name = TestLogger
appender.testlogger.type = CONSOLE
-appender.testlogger.target = SYSTEM_ERR
+appender.testlogger.target = SYSTEM_OUT
Review Comment:
Any reason to change this?
##########
flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/common/schema/PulsarSchemaTypeInformation.java:
##########
@@ -66,8 +66,10 @@ public boolean isKeyType() {
return false;
}
+ // https://issues.apache.org/jira/browse/FLINK-34125 modified the
definition, BTW the param is
+ // useless for the current project.
Review Comment:
Omit this comment.
##########
.github/workflows/weekly.yml:
##########
@@ -30,8 +30,9 @@ jobs:
if: github.repository_owner == 'apache'
strategy:
matrix:
+ jdk: [ 11, 17 ]
Review Comment:
Let's move this under `flink_braches`, where version is `2.0.2`, cause the
`v4.2` branch still needs JDK8 to compile against.
##########
flink-connector-pulsar/src/test/resources/log4j2-test.properties:
##########
@@ -22,7 +22,7 @@ rootLogger.level=INFO
rootLogger.appenderRef.test.ref=TestLogger
appender.testlogger.name=TestLogger
appender.testlogger.type=CONSOLE
-appender.testlogger.target=SYSTEM_ERR
+appender.testlogger.target=SYSTEM_OUT
Review Comment:
Any reason to change this?
##########
pom.xml:
##########
@@ -76,10 +77,13 @@ under the License.
<commons-compress.version>1.26.1</commons-compress.version>
<commons-lang3.version>3.14.0</commons-lang3.version>
<commons-io.version>2.15.1</commons-io.version>
+ <caffeine.version>2.9.1</caffeine.version>
+ <checker-qual.version>3.12.0</checker-qual.version>
+ <errorprone.version>2.11.0</errorprone.version>
<byte-buddy.version>1.12.20</byte-buddy.version>
<kryo.version>2.24.0</kryo.version>
<objenesis.version>3.3</objenesis.version>
- <jackson-bom.version>2.13.4.20221013</jackson-bom.version>
+ <jackson-bom.version>2.18.6</jackson-bom.version>
Review Comment:
Latest 2.x Jackson version is 2.22.2, any reason to not use that?
##########
flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/sink/committer/PulsarCommittable.java:
##########
@@ -18,14 +18,14 @@
package org.apache.flink.connector.pulsar.sink.committer;
-import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.PublicEvolving;
import org.apache.pulsar.client.api.transaction.TxnID;
import java.util.Objects;
/** The writer state for Pulsar connector. We would used in Pulsar committer.
*/
-@Internal
+@PublicEvolving
Review Comment:
Why changing this to `@PublicEvolving`? I think this should stay `@Internal`.
##########
flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/sink/PulsarSink.java:
##########
@@ -141,10 +147,10 @@ public PrecommittingSinkWriter<IN, PulsarCommittable>
createWriter(InitContext i
initContext);
}
- @Internal
Review Comment:
Why remove the `@Internal`? I think this should stay `@Internal`.
##########
flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/common/schema/PulsarSchemaTypeSerializer.java:
##########
@@ -200,9 +200,11 @@ public TypeSerializer<T> restoreSerializer() {
return new PulsarSchemaTypeSerializer<>(schema);
}
+ // https://issues.apache.org/jira/browse/FLINK-30614 changed the
definition, BTW the param
+ // is useless for the current project.
Review Comment:
Omit this comment.
--
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]
