changed some tests thread array list into executor and added timeout … #149
Conversation
…and exception handling like asked in issue #142
There was a problem hiding this comment.
Hi @OrHayat.
Thank you for your contribution.
It seems that your implementation is inlined with what we intended in #142.
However, it neglects to propagate the exceptions.
This was the main motivation behind this issue.
The idea to keep the Future objects in some collection, and call .get() for each of the futures once the test ends.
This will raise an exception if the thread raised an exception.
So the test will fail if one of the threads had an error.
…and exception handling like asked in issue #142
|
|
Alright i fixed those things espically the error propergation with Future objects and in order to it to work you need to use callables instead of runnables (which cant propergate the errors in other threads) which i changed |
There was a problem hiding this comment.
Thanks. I tested your solution and it seems to work well.
Please attend to the minor issues before we can merge this PR.
In addition, pay attention to styling issues such as spacing before and after operators (e.g., "==", "=", "&&", etc.), redundant spaces and so on.
These can automatically be solved using Intellij (or any other IDE) auto-format feature.
| long timeToWait = Math.max(10, | ||
| timeLimitInMs-Duration.between(startingTime, currentTime).toMillis()); | ||
| task.get(timeToWait, TimeUnit.MILLISECONDS); | ||
| it.remove(); |
There was a problem hiding this comment.
Shouldn't we remove the item only if get() was successful?
There was a problem hiding this comment.
no because if get faill throw an exception which the function throw away i added catch finally to force close the executor in any case and to print the error which might not be wanted.
There was a problem hiding this comment.
I see. Please add a comment above it.remove() explaining this flow.
core/src/test/java/com/yahoo/oak/NativeMemoryAllocatorTest.java
Outdated
Show resolved
Hide resolved
…and exception handling like asked in issue #142
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.