Michael Ho has posted comments on this change. Change subject: IMPALA-3242: Remove most usages of RuntimeState::SetMemLimitExceeded() ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/hdfs-rcfile-scanner.cc File be/src/exec/hdfs-rcfile-scanner.cc: Line 267: RC file > RCFile scanner Done http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 1189: ()) > should this also skip for IsMemLimitExceeded()? Okay to leave as is though I prefer to keep it as is. The reason I put this check here is that scanner threads which picked up cancellation due to other threads' failure will unnecessarily log that they hit parse error and that's really confusing given that they didn't really hit any error. http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 1149: int64_t alloc_size = PAGG_DEFAULT_HASH_TABLE_SZ * HashTable::BucketSize(); > I'm not a big fan of using the initial size here, since it means changes el The hash table hasn't been created yet before InitHashTable() is called and it will set the number of buckets to zero if it fails to initialize so there is not exactly a clean way to record the alloc size and it seems silly to always record the alloc size of InitHashTable() in a variable somewhere for the rare case of failures. InitiHashTable() also uses PAGG_DEFAULT_HASH_TABLE_SZ for the initial sizing of the hash table so changes there will be automatically reflected here. I will add a remark in InitHashTable() about the need to update the error message if anything changes there. Line 1251: UNLIKELY > I don't think this path is perf-critical enough to justify the annotations. It doesn't hurt to put UNLIKELY() on uncommon error cases to streamline the common case. Line 1252: status > Dead variable. Oops...Removed. http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 636: UNLIKELY > Again, not sure this is hot enough to justify annotations. Putting UNLIKELY() on uncommon case helps streamline the code for more common case. This is on the critical path for runtime filter generation so I guess every little bit helps. Line 862: return mem_tracker()->MemLimitExceeded(state, details, 0); > Change to a different error to be consistent with the aggregation? I think the aggregation code also uses the same error message so they are consistent, no ? That said, I converted here and PAGG to use new error codes instead in the new patch. http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 532: query > actually, isn't it both query and process? Done Line 533: . > by MemTracker::MemLimitExceeded(). Done http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 243: /// generic "Memory limit exceeded" error. > let's add a note here saying this is depricated and instead MemTracker::Mem Done -- To view, visit http://gerrit.cloudera.org:8080/3140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic0ca128c768d1e73713866e8c513a1b75e6b4b59 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
