impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
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:

(12 comments)

LMK if i missed responding to anything.

http://gerrit.cloudera.org:8080/#/c/5801/27/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

Line 236:   Status ClaimBufferReservation(RuntimeState* state);
> ClaimBufferReservation()?
WFM


http://gerrit.cloudera.org:8080/#/c/5801/26/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

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: http://impala.apache.org/docs/build/ht
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?


http://gerrit.cloudera.org:8080/#/c/5801/27/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

PS27, Line 637: 
> Good point. BTSV2 uses the MAX_ROW_SIZE error code, so I should use that he
nice


http://gerrit.cloudera.org:8080/#/c/5801/28/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

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_; }
same?


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 http://gerrit.cloudera.org:8080/5801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Mime
View raw message