Michael Ho has posted comments on this change. Change subject: IMPALA-3946: fix MemPool integrity issues with empty chunks ......................................................................
Patch Set 1: (4 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 transferred. Whether the chunk is transferred should have nothing to do whether the chunk is empty or not, right ? 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 more explicit: if (UNLIKELY(current_chunk_idx_ == -1)) { first_free_idx = 0; } else { DCHECK_GE(current_chunk_idx, 0); first_free_idx = current_chunk_idx_ + (chunks_[current_chunk_idx_] > 0); // Always start at current_chunk_idx + 1 as we know the current chunk cannot fit. for (int idx = current_chunk_idx_ + 1; idx < chunks_.size(); ++idx) { .... } } It would be great to also document that the reason there can be a list of free chunks after the current chunk is a result of calling Clear() in the past. That always confuses me when I read this function without reading other functions at the same time. PS1, Line 215: false Is there a reason why this cannot be !keep_current || src->GetFreeOffset() > 0 ? PS1, Line 260: if (current_chunk_empty) Can we keep the stricter check we had before if we modify line 215 ? -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
