ARROW-12097: [C++] Modify BackgroundGenerator so it creates fewer threads#9808
ARROW-12097: [C++] Modify BackgroundGenerator so it creates fewer threads#9808westonpace wants to merge 1 commit intoapache:masterfrom
Conversation
westonpace
force-pushed
the
feature/arrow-12097
branch
from
5119e62 to
f28735b
Compare
westonpace
force-pushed
the
feature/arrow-12097
branch
from
f28735b to
3aff42e
Compare
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Just a couple minor comments.
westonpace
force-pushed
the
feature/arrow-12097
branch
4 times, most recently
from
6a7bad4 to
23505c8
Compare
There was a problem hiding this comment.
It looks like this sometimes fails on Windows.
[ RUN ] BackgroundGeneratorTests/BackgroundGeneratorTestFixture.BadResult/1
C:/projects/arrow/cpp/src/arrow/util/async_generator_test.cc(701): error: Failed
'_error_or_value70.status()' failed with Invalid: XYZ
[ FAILED ] BackgroundGeneratorTests/BackgroundGeneratorTestFixture.BadResult/1, where GetParam() = true (0 ms)
The previous call does restart the background thread, so it's possible it'll get to the failing value in this call.
There was a problem hiding this comment.
If it restarts it will restart on the call for 2 and the call that restarts can never fail in this test (because it doesn't restart until after it's grabbed a result from the queue).
I think what is happening is things are slowing down in just the right way so the background reader never actually fills the queue and so 5 is able to sneak in before 2 is asked for.
Either way, the fix is the same, I now allow the call for 2 to fail. Thanks for catching this.
…n a thread task per execution ARROW-12097: WIP on new background generator ARROW-12097: Forgot to switch the async file reader over to the new background reader ARROW-12097: Added check to make sure we are handling invalid result from spawn. Added some test cases ARROW-12097: Removing leftover comment ARROW-12097: Fixed another spot where we were ignoring a Status return value ARROW-12097: Subtle race condition hidden by using Future<>.is_valid when it was better to just use an optional ARROW-12097: The BadResult test could fail under certain timing conditions. I needed to loosen up how the check interpreted success.
westonpace
force-pushed
the
feature/arrow-12097
branch
from
ad993ad to
c169e34
Compare
|
The integration job looks like a flake. I restarted the Travis job. |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Travis is still being odd but it looks unrelated to the PR (tests pass and it hangs afterwards).
2 participants
No description provided.