MINOR: make sure all dir jobs are completed #9728
Conversation
| * @param jobs jobs | ||
| * @return true if all pass. Otherwise, false | ||
| */ | ||
| private[log] def allPass(jobs: Seq[Future[_]]): Boolean = { |
There was a problem hiding this comment.
The name makes this look generic, but it's not. The logging is very specific. I would change the name to be specific.
There was a problem hiding this comment.
How about "waitForAllToComplete"
There was a problem hiding this comment.
The log message talks explicitly about LogManager shutdown. Seems like you'd want it to say something about log manager shutdown. I would even set the type parameter to the type we expect from the shutdown method.
There was a problem hiding this comment.
I would even set the type parameter to the type we expect from the shutdown method.
There is no explicit type so I didn't define the type parameter.
|
@ijuma Could you take a look? |
| def testWaitForAllToComplete(): Unit = { | ||
| var invokedCount = 0 | ||
| val success: Future[Boolean] = Mockito.mock(classOf[Future[Boolean]]) | ||
| Mockito.when(success.get()).thenAnswer(_ => invokedCount += 1) |
There was a problem hiding this comment.
Should we not return true in this answer?
There was a problem hiding this comment.
waitForAllToComplete does not use the return value so it does not cause casting error even though we don't return true. However, it does look like a bug so I will add the return value.
revert the change introduced by #9680 and add a unit test to verify
Committer Checklist (excluded from commit message)