Dan Hecht has posted comments on this change. Change subject: IMPALA-3936: BufferedBlockMgr fixes for Pin() while write in flight. ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/3832/2//COMMIT_MSG Commit Message: Line 14: the in-flight write to complete before transferring the buffer. TransferBlock() will either unpin or delete. In the delete case, WriteComplete() should finish the delete, so we should be okay, right? In the unpin case, it looks like we will hit the DCHECK in WriteUnpinnedBlock() that !block->in_write_. Is that what's happening? Or is there some other corruption? Line 17: woken up. The fix is to clarify when condition variables will be was this manifesting in a hang? PS2, Line 19: right write Line 20: notify_all() in case there are multiple threads waiting. is using notify_all() necessary, or just done for consistency? Line 28: It succeeded. is this a configuration that will get coverage at some point during releases (i.e. do we have good regression coverage)? http://gerrit.cloudera.org:8080/#/c/3832/2/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: Line 929: block->write_complete_cv_.notify_all(); seems okay to use notify_all(), but in practice why would there be multiple threads waiting on this cv? It's only used by TransferBlock() and two threads shouldn't be trying to do that, right? http://gerrit.cloudera.org:8080/#/c/3832/2/be/src/runtime/buffered-block-mgr.h File be/src/runtime/buffered-block-mgr.h: Line 449: /// needs to not have been taken when this function is called. looks like this comment needs updating if we'll always wait for in-flight writes. -- To view, visit http://gerrit.cloudera.org:8080/3832 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4be4fad8e6f2303db19ea1e2bd0f13523781ae8e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
