[Native][Issue-3593] Handle Task::updateBroadcastOutputBuffer failure gracefully with error log message#18939
Conversation
karteekmurthys
requested review from
aditi-pandit,
majetideepak and
mbasmanova
There was a problem hiding this comment.
Please remove this commented code.
There was a problem hiding this comment.
Why is the condition BufferType::BROADCAST removed ? That doesn't seem right to me.
There was a problem hiding this comment.
Is it possible to return and test a boolean for this method instead ? Returning nullptr for a successful update vs TaskInfo for a failure seems weird.
There was a problem hiding this comment.
Why are you always calling updateBroadcastOutputBuffers ? It seems more readable to check the BROADCAST condition here than in the function called ?
There was a problem hiding this comment.
It doesn't seem like you are using prestoTask in this function. Why are you passing it ?
karteekmurthys
force-pushed
the
task-manager-race-condition
branch
3 times, most recently
from
bd1871e to
43e3d59
Compare
|
@prestobot kick off tests |
karteekmurthys
force-pushed
the
task-manager-race-condition
branch
from
87c65f4 to
4e654b9
Compare
|
@mbasmanova @aditi-pandit would you please review this. |
1 similar comment
|
@mbasmanova @aditi-pandit would you please review this. |
karteekmurthys
force-pushed
the
task-manager-race-condition
branch
from
797ca02 to
d4c0e18
Compare
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@karteekmurthys The change looks good to me. Thanks. Please, update commit message to provide more details about this change.
No testing required as we are not taking any action for error case.
Not sure if this is a good reason for not adding a test.
karteekmurthys
force-pushed
the
task-manager-race-condition
branch
2 times, most recently
from
8003838 to
b1a295b
Compare
karteekmurthys
changed the title
aditi-pandit
changed the title
+1. Your change has an effect on the behavior. It would be good to mock and check that you caught the error. Please try to address that in another PR. |
4 participants
The TaskManager may enter a race condition on Task::updateBrodacastBuffers() leading to an exception:"Output buffers not found" if the PartitionedOutputBuffers for that task are previously deleted by a downstream driver.
This PR handles this path gracefully by checking the error condition in a new
PartitionedOutputBuffersManager::getBufferIfExists()API instead ofgetBuffer()call (which throws that exception). An error message is also logged in this case.