Tim Armstrong has posted comments on this change. Change subject: IMPALA-3946: fix MemPool integrity issues with empty chunks ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/3838/1/be/src/runtime/mem-pool-test.cc File be/src/runtime/mem-pool-test.cc: PS1, Line 435: Empty chunk > May be I misunderstood it but I think you meant the chunk from src isn't tr Yeah, reworded to "Src chunk shouldn't be transferred." PS1, Line 442: make > making Done PS1, Line 456: resuled > resulted Done PS1, Line 463: make > making Done http://gerrit.cloudera.org:8080/#/c/3838/1/be/src/runtime/mem-pool.cc File be/src/runtime/mem-pool.cc: PS1, Line 105: int first_free_idx = 0; > IMHO, It may be slightly clearer to make the assignment of this variable mo Done PS1, Line 215: false > Is there a reason why this cannot be !keep_current || src->GetFreeOffset() See below comment. PS1, Line 260: if (current_chunk_empty) > Can we keep the stricter check we had before if we modify line 215 ? The unit tests I added hit the old DCHECK in a lot of other places where it was calling CheckIntegrity() when entering the method (since by calling ReturnPartialAllocation() you can get it into a state where the current chunk is empty). At all callsites with current_chunk_empty=false the stricter check was not applicable, so we'd have to call this function with current_chunk_empty = (CurrentChunkOffset() == 0), which just disables the stricter check. -- To view, visit http://gerrit.cloudera.org:8080/3838 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03ad12e5b2b63cbb55e5c52562416d73a4bd9346 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
