You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/02/01 16:30:27 UTC
[GitHub] [hive] pgaref opened a new pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
pgaref opened a new pull request #1933:
URL: https://github.com/apache/hive/pull/1933
### What changes were proposed in this pull request?
Apply Sane Default for Tez Containers as Last Resort
### Why are the changes needed?
If Tez Container Size is an invalid value ( <= 0 ) then it falls back onto the MapReduce configurations, but if the MapReduce configurations have invalid values ( <= 0 ), they are excepted regardless and this will cause failures down the road.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568030343
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", (maxExecutorMemory / 1024L * 1024L));
Review comment:
Thanks, however `/ 1024L * 1024L` is a no-op
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-770985003
Similar simplification can be applied in DagUtils getContainerResource method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-770990643
> I think all the changes should be rolled into `getContainerResource` and then everything else calls that.
Sure, I guess you mean always returning a Resource instance, and use memory or cpu wherever needed ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568066289
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", maxExecutorMemory / (1024L * 1024L));
Review comment:
This just does not look correct to me.
Are you trying to log the value here in MB? I think `this.maxExecutorMemory` is in MB already. It is currently TB to MB with this large divisor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-771865428
Hey @belugabehr -- are we happy with the changes? Shall we push this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r569580552
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -34,9 +37,8 @@
private Configuration conf;
private boolean isTez;
private boolean isLlap;
- private long maxExecutorMemory;
- private long mapJoinMemoryThreshold;
- private long dynPartJoinMemoryThreshold;
+ private long maxExecutorMemory; // value in Bytes
Review comment:
Sorry to nit, but can we make these 'final' instance variables? Also, can you please move the 'value in bytes' into a proper Javadoc on the getter method?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java
##########
@@ -700,13 +700,26 @@ public int getPartition(Object key, Object value, int numPartitions) {
* container size isn't set.
*/
public static Resource getContainerResource(Configuration conf) {
- int memory = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE) > 0 ?
- HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE) :
- conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB);
- int cpus = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCPUVCORES) > 0 ?
- HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCPUVCORES) :
- conf.getInt(MRJobConfig.MAP_CPU_VCORES, MRJobConfig.DEFAULT_MAP_CPU_VCORES);
- return Resource.newInstance(memory, cpus);
+ int memorySizeMb = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
+ if (memorySizeMb <= 0) {
+ LOG.warn("Falling back to MapReduce container MB {}", MRJobConfig.MAP_MEMORY_MB);
+ memorySizeMb = conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB);
+ // When config is explicitly set to "-1" defaultValue does not work!
+ if (memorySizeMb <= 0) {
+ LOG.warn("Falling back to default container MB {}", MRJobConfig.DEFAULT_MAP_MEMORY_MB);
+ memorySizeMb = MRJobConfig.DEFAULT_MAP_MEMORY_MB;
+ }
+ }
+ int cpuCores = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCPUVCORES);
+ if (cpuCores <= 0) {
+ LOG.warn("Falling back to MapReduce container VCores {}", MRJobConfig.MAP_CPU_VCORES);
Review comment:
Can we please update to say:
```java
LOG.warn("No Tez VCore size specified by {}. Falling back...", HiveConf.ConfVars.HIVETEZCPUVCORES, MRJobConfig.MAP_CPU_VCORES);
```
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java
##########
@@ -700,13 +700,26 @@ public int getPartition(Object key, Object value, int numPartitions) {
* container size isn't set.
*/
public static Resource getContainerResource(Configuration conf) {
- int memory = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE) > 0 ?
- HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE) :
- conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB);
- int cpus = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCPUVCORES) > 0 ?
- HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCPUVCORES) :
- conf.getInt(MRJobConfig.MAP_CPU_VCORES, MRJobConfig.DEFAULT_MAP_CPU_VCORES);
- return Resource.newInstance(memory, cpus);
+ int memorySizeMb = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
+ if (memorySizeMb <= 0) {
+ LOG.warn("Falling back to MapReduce container MB {}", MRJobConfig.MAP_MEMORY_MB);
Review comment:
Can we please update to say:
```java
LOG.warn("No Tez container size specified by {}. Falling back...", HiveConf.ConfVars.HIVETEZCONTAINERSIZE, MRJobConfig.MAP_MEMORY_MB);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568023927
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", (maxExecutorMemory / 1024L * 1024L));
Review comment:
maxExecutorMemory is in bytes (see how we handle it on Tez and MR mode) -- however LlapCluster state does the conversion prematurely as part of: https://github.com/apache/hive/blob/5eebbdf7c5750b31e1c43fe576fc0ab728bce05c/ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java#L147
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-770988991
I think all the changes should be rolled into `getContainerResource` and then everything else calls that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref merged pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref merged pull request #1933:
URL: https://github.com/apache/hive/pull/1933
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-771865428
Hey @belugabehr -- are we happy with the changes? Shall we push this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568000803
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -53,11 +54,16 @@ public MemoryInfo(Configuration conf) {
this.maxExecutorMemory = memPerInstance / numExecutors;
}
} else if (isTez) {
- int containerSizeMb = StatsUtils.getAvailableMemory(conf, true);
+ long containerSizeMb = DagUtils.getContainerResource(conf).getMemorySize();
float heapFraction = HiveConf.getFloatVar(conf, HiveConf.ConfVars.TEZ_CONTAINER_MAX_JAVA_HEAP_FRACTION);
this.maxExecutorMemory = (long) ((containerSizeMb * 1024L * 1024L) * heapFraction);
} else {
- this.maxExecutorMemory = StatsUtils.getAvailableMemory(conf, false) * 1024L * 1024L;
+ this.maxExecutorMemory =
+ conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB) * 1024L * 1024L;
+ // this can happen when config is explicitly set to "-1", in which case defaultValue also does not work
+ if (maxExecutorMemory < 0) {
+ maxExecutorMemory = MRJobConfig.DEFAULT_MAP_MEMORY_MB * 1024L * 1024L;
Review comment:
Log message please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r567969784
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java
##########
@@ -1915,15 +1915,26 @@ private static String getFullyQualifiedName(String... names) {
return result;
}
- public static long getAvailableMemory(Configuration conf) {
- int memory = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
- if (memory <= 0) {
- memory = conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB);
- if (memory <= 0) {
- memory = 1024;
+ /**
+ * Get the Container Memory Size from given conf (in MB).
+ * Returns HIVETEZCONTAINERSIZE when set, otherwise falls back to MAP_MEMORY_MB.
+ * When MAP_MEMORY_MB is explicitly set to "-1" uses DEFAULT_MAP_MEMORY_MB (1024) to avoid failures.
+ * @param conf Configuration
+ * @param isTez true if in Tez mode
+ * @return Container Memory Size in MB
+ */
+ public static int getAvailableMemory(Configuration conf, boolean isTez) {
+ int containerMemSizeMb = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
Review comment:
The reason is I reused the logic for MR mode: https://github.com/apache/hive/pull/1933/files#diff-e3956e96fab0a8e9604b0e484a2ee7db29b83a62fdae4d08de068e4712191663L67
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568020306
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", (maxExecutorMemory / 1024L * 1024L));
Review comment:
I think there's a type here, maxExecutorMemory I believe is already is already in MB? Regardless, there's a typo here... I think you want to multiple 1024 twice, not divide :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-855489341
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r567967118
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java
##########
@@ -1915,15 +1915,26 @@ private static String getFullyQualifiedName(String... names) {
return result;
}
- public static long getAvailableMemory(Configuration conf) {
- int memory = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
- if (memory <= 0) {
- memory = conf.getInt(MRJobConfig.MAP_MEMORY_MB, MRJobConfig.DEFAULT_MAP_MEMORY_MB);
- if (memory <= 0) {
- memory = 1024;
+ /**
+ * Get the Container Memory Size from given conf (in MB).
+ * Returns HIVETEZCONTAINERSIZE when set, otherwise falls back to MAP_MEMORY_MB.
+ * When MAP_MEMORY_MB is explicitly set to "-1" uses DEFAULT_MAP_MEMORY_MB (1024) to avoid failures.
+ * @param conf Configuration
+ * @param isTez true if in Tez mode
+ * @return Container Memory Size in MB
+ */
+ public static int getAvailableMemory(Configuration conf, boolean isTez) {
+ int containerMemSizeMb = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVETEZCONTAINERSIZE);
Review comment:
Not sure why `isTez` was added here,... why return anything if it's not Tez as this method is strictly for determining Tez container size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-860077089
This has been siting for a while, shall we push this @belugabehr ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-772699104
> @pgaref If only you hadn't asked me to look one more time I probably would have just merged. :)
Np at all @belugabehr -- thats what reviews are for :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568080013
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", maxExecutorMemory / (1024L * 1024L));
Review comment:
maxExecutorMemory should be in Bytes, see
https://github.com/apache/hive/blob/aee31f8d03a9d9b1fce3b5cc8788b2238cbaf351/ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java#L106
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568080013
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", maxExecutorMemory / (1024L * 1024L));
Review comment:
maxExecutorMemory should be in Bytes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] belugabehr commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568002349
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
##########
@@ -1734,7 +1735,7 @@ private boolean checkMapSideAggregation(GroupByOperator gop,
float hashAggMaxThreshold = conf.getFloatVar(HiveConf.ConfVars.HIVEMAPAGGRMEMORYTHRESHOLD);
// get available map memory
- long totalMemory = StatsUtils.getAvailableMemory(conf, true) * 1000L * 1000L;
+ long totalMemory = DagUtils.getContainerResource(conf).getMemorySize() * 1000L * 1000L;
Review comment:
Existing issue, but please address here: this should be in MiB (1024L)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568011118
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java
##########
@@ -1734,7 +1735,7 @@ private boolean checkMapSideAggregation(GroupByOperator gop,
float hashAggMaxThreshold = conf.getFloatVar(HiveConf.ConfVars.HIVEMAPAGGRMEMORYTHRESHOLD);
// get available map memory
- long totalMemory = StatsUtils.getAvailableMemory(conf, true) * 1000L * 1000L;
+ long totalMemory = DagUtils.getContainerResource(conf).getMemorySize() * 1000L * 1000L;
Review comment:
ACK
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568023927
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", (maxExecutorMemory / 1024L * 1024L));
Review comment:
maxExecutorMemory is in bytes (see how we handle it on Tez and MR mode) -- however LlapClusterState does the conversion to bytes (from MB) prematurely as part of: https://github.com/apache/hive/blob/5eebbdf7c5750b31e1c43fe576fc0ab728bce05c/ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java#L147
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref edited a comment on pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref edited a comment on pull request #1933:
URL: https://github.com/apache/hive/pull/1933#issuecomment-772699104
> @pgaref If only you hadn't asked me to look one more time I probably would have just merged. :)
Np at all @belugabehr -- thats what reviews are for :)
Updated PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pgaref commented on a change in pull request #1933: HIVE-24707: Apply Sane Default for Tez Containers as Last Resort
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #1933:
URL: https://github.com/apache/hive/pull/1933#discussion_r568034425
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/MemoryInfo.java
##########
@@ -46,30 +48,25 @@ public MemoryInfo(Configuration conf) {
llapInfo.initClusterInfo();
if (llapInfo.hasClusterInfo()) {
this.maxExecutorMemory = llapInfo.getMemoryPerExecutor();
+ LOG.info("Using LLAP registry executor MB {}", (maxExecutorMemory / 1024L * 1024L));
Review comment:
Good catch, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org