Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4833: Compute precise per-host reservation size
......................................................................


Patch Set 1:

(3 comments)

Overall I think this looks good - obviously it needs some tests but that's my 
main concern.

I played around a bit and it seemed to work. E.g. this contrived query resulted 
in different reservations per node:

    select straight_join * from tpch_parquet.lineitem join (select 
o_shippriority, count(distinct o_orderkey) from tpch_parquet.orders group by 1) 
v on o_shippriority = l_orderkey;

    Per Host Min Reservation: tarmstrong-box:22000(36.12 MB) 
tarmstrong-box:22001(36.12 MB) tarmstrong-box:22002(1.06 MB)

http://gerrit.cloudera.org:8080/#/c/7630/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 484:   6: i64 min_reservation_bytes
Are these optional or required? Not sure what the default is in thrift, we seem 
to be explicit elsewhere.


http://gerrit.cloudera.org:8080/#/c/7630/1/common/thrift/Planner.thrift
File common/thrift/Planner.thrift:

Line 66:   7: optional i64 min_reservation_bytes
It's kind-of confusing that these are optional, since the backend handles this 
case in exactly the same way as if they were zero. It might be clearer to make 
these required and set them to zero in PlanFragment.java if the profile is 
invalid. 

That way the invalid resource profile handling is more contained in 
PlanFragment. The profiles should only be invalid for join build sinks in 
parallel plans, which is explained over there.


http://gerrit.cloudera.org:8080/#/c/7630/1/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

Line 365:       // Different fragments do not synchronize their Open() and 
Close(), so the backend
I think this comment is important for understand why we sum up the fragments. 
Can we reference this in coordinator-backend-state.cc briefly?


-- 
To view, visit http://gerrit.cloudera.org:8080/7630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbcd1e9b1be14edc4017b4907e83f9d56059fbac
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to