-
Notifications
You must be signed in to change notification settings - Fork 167
[FLINK-38451] Stop logging interrupts as ERROR #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good left some minor comments.
// reset interrupt flag that is when the Kafka exception is created | ||
Thread.interrupted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since we rethrow the interrupted exception, it's not necessary to mark the thread interrupted again, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you should not even do it. Hence I reset the flag here. (This is not Thread.interrupt()).
...ector-kafka/src/main/java/org/apache/flink/connector/kafka/sink/internal/KafkaCommitter.java
Outdated
Show resolved
Hide resolved
...r-kafka/src/test/java/org/apache/flink/connector/kafka/sink/internal/KafkaCommitterTest.java
Show resolved
Hide resolved
new Thread( | ||
() -> { | ||
try { | ||
serverSocket.accept().getInputStream().read(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this also trigger by some kind of metadata request that the internal kafkaProducer does? I want to avoid interrupting before we actually call commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only listening right before commitTransaction. So any other producer initialization would be done already. However, since we use resumeTransaction with reflection hacks, we actually don't communicate with the broker at all (it's not available for this test by design).
Note that commitTransaction actually issues two requests (one is a metadata, one the actual commit). I haven't found a way or a reason to only wait on the second without relying on internals. WDYT?
Note that I reran the new test 1000 times to make sure that it's not flaky (on my machine). I also used break points inside the socket listener to force producer retries. |
71c16ca
to
c56aebe
Compare
When cancelling a job, we may interrupt the KafkaCommitter. This currently leads to ERROR log "Transaction ... encountered error and data has been potentially lost." However, that exception is expected and not leading to any data loss beyond the normal inconsistencies because of cancellation. In many cases, the commit already succeeded. Further, a job restart will lead to data being eventually committed.
c56aebe
to
ddf6e9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When cancelling a job, we may interrupt the KafkaCommitter. This currently leads to ERROR log "Transaction ... encountered error and data has been potentially lost."
However, that exception is expected and not leading to any data loss beyond the normal inconsistencies because of cancellation. In many cases, the commit already succeeded. Further, a job restart will lead to data being eventually committed.