Matthew Jacobs has posted comments on this change.

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


Patch Set 1:

(3 comments)

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 
The default is effectively optional:
https://thrift.apache.org/docs/idl#field-requiredness

While the doc says the default required-ness is good, I agree it's helpful to 
be explicit, especially in our code where we typically don't rely on the 
default. The protocol_version thing is misleading because we've been iterating 
on these interfaces regularly without bumping the version. That said, it looks 
like the convention here is to avoid explicitly requiring these fields, and 
instead indicating they're required in a particular version. (Technically 
that's a good practice, since required fields can be evil. Unfortunately we're 
pretty inconsistent.)


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 t
Done


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 fragment
Done


-- 
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