Skip to content
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

KAFKA-3865: Fix transient failure in WorkerSourceTaskTest.testSlowTaskStart #1531

Closed
wants to merge 1 commit into from

Conversation

@hachikuji
Copy link
Contributor

commented Jun 21, 2016

No description provided.

@hachikuji

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

Ping @ewencp

@Ishiihara

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

Making the changes to synchronous is less sensitive to timing. I think It should be fine as unit tests are checking the correctness for basic cases.

@Ishiihara

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

As discussed offline with @hachikuji, the addition of future.get() is to ensure that the exception thrown by the executor is propagated. Also it is not safe to throw the InterruptedException as the flag set by the exception is only cleared with next awaitLatch.

@@ -68,7 +65,6 @@

@RunWith(PowerMockRunner.class)
public class WorkerSourceTaskTest extends ThreadedTest {

This comment has been minimized.

Copy link
@ewencp

ewencp Jun 21, 2016

Contributor

Can we just drop the ThreadedTest now? We no longer have a ShutdownableThread which is what ThreadedTest relies on.

This comment has been minimized.

Copy link
@ewencp

ewencp Jun 21, 2016

Contributor

Actually, there are a few other tests that use this too. I'll file a JIRA to clean it up -- it's not reliable enough anyway (and maybe some of the tests can just drop the use of the thread and mock the necessary bits instead if that doesn't make things even messier).

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jun 21, 2016

Author Contributor

We could probably add something which provides the same capability for an executor service though. That would be better than sprinkling future.get() calls all over the place.

@ewencp

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

LGTM. Will merge to both trunk and 0.10.0 since it just cleans up the unit test and should hopefully help w/ stability of 0.10.0 builds too.

@asfgit asfgit closed this in 50781b7 Jun 21, 2016
asfgit pushed a commit that referenced this pull request Jun 21, 2016
…kStart

Author: Jason Gustafson <jason@confluent.io>

Reviewers: Liquan Pei <liquanpei@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #1531 from hachikuji/KAFKA-3865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.