KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener#8857
Merged
Merged
Conversation
kowshik
changed the title
Member
|
ok to test |
chia7712
reviewed
Jun 12, 2020
kowshik
commented
Jun 12, 2020
Contributor
Author
|
@chia7712 Could you please help trigger a CI run? |
Member
|
retest this please |
Contributor
Author
|
@chia7712 I think CI is not triggering. I'd appreciate the help if you could trigger it again. |
Member
|
Is this CI not working for this PR? https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2932/console |
Member
|
CI comes back :) |
junrao
reviewed
Jun 12, 2020
3 tasks
kowshik
force-pushed
the
fix_test_KAFKA-10157
branch
from
June 12, 2020 17:32
6e94ead to
0b296d8
Compare
kowshik
commented
Jun 12, 2020
Contributor
|
test this please |
Contributor
|
ok to test |
abbccdda
reviewed
Jun 12, 2020
| // a concurrent call to FinalizedFeatureChangeListener.close() could interrupt the thread | ||
| // and cause an InterruptedException to be raised from queue.take(). In such a case, it is | ||
| // safe to ignore the exception if the thread is being shutdown. We raise the exception | ||
| // here again, because, it is ignored by ShutdownableThread if it is shutting down. |
There was a problem hiding this comment.
nit: if it is shutting down -> when shutting down.
apovzner
approved these changes
Jun 12, 2020
Contributor
There was a problem hiding this comment.
LGTM. Also, I see that PR build does not have the failures that were happening before this PR (reported in KAFKA-10157), i.e., I don't see a failure like "finished with non-zero exit value 1" in console output.
Contributor
Author
|
@apovzner Can you go ahead and merge this PR? The latest failures in CI are unrelated. |
Kvicii
pushed a commit
to Kvicii/kafka
that referenced
this pull request
Jun 13, 2020
* 'trunk' of github.com:apache/kafka: (42 commits) HOTFIX: Fix compile error in TopicAdminTest (apache#8866) KAFKA-10144: clean up corrupted standby tasks before attempting a commit (apache#8849) KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener (apache#8857) KAFKA-9432: automated protocol for DescribeConfigs (apache#8312) KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764) KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680) KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787) KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818) KAFKA-9374: Make connector interactions asynchronous (apache#8069) MINOR: reduce sizeInBytes for percentiles metrics (apache#8835) KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829) KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828) KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455) KAFKA-7833: Add missing test (apache#8847) KAFKA-9066: Retain metrics for failed tasks (apache#8502) KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453) KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663) KAFKA-9441: remove prepareClose() to simplify task management (apache#8833) KAFKA-7833: Add Global/StateStore name conflict check (apache#8825) KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the cause of failing tests mentioned in the jira:
kafka.network.DynamicConnectionQuotaTestkafka.api.CustomQuotaCallbackTestkafka.server.DynamicBrokerReconfigurationTestIssue:
The call to
ChangeNotificationProcessorThread.queue.take()could throw anInterruptedException. While the queue is empty and the thread is blocking on taking an item from the queue, a concurrent call toFinalizedFeatureChangeListener.close()could interrupt the thread and cause anInterruptedExceptionto be raised fromqueue.take(). In such a case, it is safe to ignore the exception since the thread is being shutdown.Definitely ignoring the
InterruptedExceptionfor the above reason was the intent of the code that used theignoringclause for the same. But it seems unfortunately theignoringclause does not ignoreInterruptedException, so that doesn't work for us. To confirm this theory, I found the following code inscala.util.control.Exception.scala: https://github.com/scala/scala/blob/v2.12.0/src/library/scala/util/control/Exception.scala#L167-L176.Fix:
The fix in this PR is to just not use the
ignoringclause. We rely on existing mechanism inShutdownableThreadthat ignores the exception during shutdown.Test plan:
Ran the unit and integration tests and found that the test failures are gone now.
I will wait for CI to pass before merging this PR.