mimaison commented on code in PR #18074:
URL: https://github.com/apache/kafka/pull/18074#discussion_r1877818616
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ExactlyOnceWorkerSourceTask.java:
##########
@@ -321,7 +321,7 @@ private void commitTransaction() {
error = flushError.get();
if (error != null) {
- recordCommitFailure(time.milliseconds() - started, null);
+ recordCommitFailure(time.milliseconds() - started);
Review Comment:
This changes the behavior of `recordCommitFailure()`. When calling the
method with 2 arguments, it then calls `recordCommit()` with `success` set to
`false` which in turn impacts metrics.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ConnectorValidationIntegrationTest.java:
##########
@@ -549,12 +549,7 @@ public ConfigDef config() {
}
}
- public static class TestConverterWithNoConfigDef extends TestConverter {
- @Override
- public ConfigDef config() {
- return null;
- }
- }
+ public static class TestConverterWithNoConfigDef extends TestConverter { }
Review Comment:
While it copes the behavior the the super class, I'd prefer keeping the
current code to keep things easy to understand.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSourceTask.java:
##########
@@ -262,11 +262,11 @@ public boolean commitOffsets() {
shouldFlush = offsetWriter.beginFlush(timeout -
time.milliseconds(), TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
log.warn("{} Interrupted while waiting for previous offset flush
to complete, cancelling", this);
- recordCommitFailure(time.milliseconds() - started, e);
+ recordCommitFailure(time.milliseconds() - started);
Review Comment:
Ditto
##########
connect/runtime/src/main/java/org/apache/kafka/connect/tools/PredicateDoc.java:
##########
@@ -53,29 +52,29 @@ private <P extends Predicate<?>> DocInfo(Class<P>
predicateClass, String overvie
.sorted(Comparator.comparing(docInfo -> docInfo.predicateName))
.collect(Collectors.toList());
- private static void printPredicateHtml(PrintStream out, DocInfo docInfo) {
- out.println("<div id=\"" + docInfo.predicateName + "\">");
+ private static void printPredicateHtml(DocInfo docInfo) {
Review Comment:
I wonder if we should instead switch these method to be `toHtml()` like in
other module and have them return a String. That way it's testable (I think the
idea of passing a `PrintStream` was to make it easily testable) and consistent
with other modules. WDYT?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSinkTask.java:
##########
@@ -282,7 +282,7 @@ private void onCommitCompleted(Throwable error, long seqno,
Map<TopicPartition,
log.error("{} Commit of offsets threw an unexpected exception
for sequence number {}: {}",
this, seqno, committedOffsets, error);
commitFailures++;
- recordCommitFailure(durationMillis, error);
+ recordCommitFailure(durationMillis);
Review Comment:
Why are you removing the error?
--
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]
