impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4674: Part 2: port backend exec to BufferPool
Date Wed, 12 Jul 2017 16:56:23 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool

Patch Set 28:


LMK if i missed responding to anything.
File be/src/exec/exec-node.h:

Line 236:   Status ClaimBufferReservation(RuntimeState* state);
> ClaimBufferReservation()?
File be/src/runtime/

PS26, Line 149:   bytes_limit = query_options().mem_limit;
> Ah good point. It looks like we do allow you to start up Impala without a p
I agree it seems best to disallow it but I'm a little worried about breaking that without
a warning period. in that case, do we not sure set the process limit and so this can just
be DCHECK_GT(mem_limit, 0) or something?

Line 152:   }
> It doesn't look like we document it:
Or buffer_pool_limit?  from the user perspective, isn't it the limit on the query's buffer
pool consumption, similar to how mem_limit is the limit on the overall memory consumption
for the query?

PS26, Line 157: 
> I don't think we have any tests that confirm that queries execute successfu
its not that important to regression test, it just seems like a weird case. maybe just do
a quick manual test to verify things are sane?
File be/src/runtime/

PS27, Line 637: 
> Good point. BTSV2 uses the MAX_ROW_SIZE error code, so I should use that he
File be/src/runtime/

PS28, Line 63: client
that's not a variable so remove quotes

Line 88:     DCHECK_LE(len, BytesRemaining());
dheck data_?

Line 97:     DCHECK_LE(len, valid_data_len_);
dcheck(data_) to document the state machine?

Line 102:   int64_t BytesRemaining() { return len() - valid_data_len_; }

Line 115:   /// to pin the page.
I think it would help to explain the state machine clearer. i.e. that WaitForBuffer() has
to be called after this before calling data(), AllocateBytes() etc

Line 130:   uint8_t* data() const { return data_; }
DCHECK(data_ != NULL) << "must be in memory " to document the expectation that WaitForBuffer()
is called first if it had been unpinned??

Line 1023:   curr_page->Destroy(sorter_->buffer_pool_client_);
its a little weird that we call something Destroy() on the page but leave it in the array.
it's okay since we're not destroying the Page* itself, but hte underlying BP page, but it
might be confusing to read. maybe at least update the function comments to clarify that distinction.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e
Gerrit-PatchSet: 28
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message