-
Notifications
You must be signed in to change notification settings - Fork 2k
remove some of thread.sleeps in test cases #1786
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
remove some of thread.sleeps in test cases #1786
Conversation
leaderElectionWorker.submit( | ||
() -> { | ||
leaderElector.run(() -> {}, () -> {}); | ||
}); | ||
// TODO: Remove this sleep | ||
Thread.sleep(Duration.ofSeconds(2).toMillis()); | ||
while (!sem.tryAcquire()) { |
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.
Is there a reason for this loop? I would remove all this code and just do:
sem.acquire()
like you did above.
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.
i wanted to print message on why we are waiting..
but i have changed semaphores to countdown latch, as i thought it would be better than sempahore in these situations
|
||
while (!sem.tryAcquire(2)) { |
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.
As above, why not just sem.acquire(2)
?
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.
wanted to print message on why we are waiting..
but i have changed semaphores to countdown latch, as i thought it would be better than sempahore in these situations
Thanks for the PR! Couple of small comments then I think this is ready to merge. |
/retest |
@karthikkondapally: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -103,7 +103,6 @@ private boolean waitForAdded(DefaultDelayingQueue queue, int size) { | |||
} | |||
|
|||
private boolean waitForWaitingQueueToFill(DefaultDelayingQueue queue) { | |||
return Wait.poll( |
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.
I don't think these are the same test. (and it seems unrelated) can you revert this change?
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.
changed it back...
/lgtm Thanks for the PR! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, karthikkondapally The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
trying to remove some of Thread.sleep()
#1223
Update:
(Some of classes dont have Thread.sleep like ReflectorRunnableTest.java)
Some Thread.sleep(2000) are because of wiremock.stubFor wiremock/wiremock#97