Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464222619
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/AbstractHashBinaryRecordBatch.java:
##
@@ -1312,7 +1312,9 @@ private void cleanup() {
}
// clean (and deallocate) each partition, and delete its spill file
for (HashPartition partn : partitions) {
- partn.close();
+ if (partn != null) {
+partn.close();
+ }
Review Comment:
(partn != null) are necessary ,see Comment above on 1. fix idea
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
### 2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
```
//1. add isException parameter when construct HashPartition object
HashPartition(FragmentContext context, BufferAllocator allocator,
ChainedHashTable baseHashTable,
RecordBatch buildBatch, RecordBatch probeBatch,
boolean semiJoin,
int recordsPerBatch, SpillSet spillSet, int partNum,
int cycleNum, int numPartitions , boolean **isException** )
//2. catch except to ensure HashPartition object has been created
} catch (OutOfMemoryException oom) {
//do not call close ,do not throw except
isException =true;
}
//3.deal with exception
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions,**isException** );
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
### 2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
```
//add isException parameter when construct HashPartition object
HashPartition(FragmentContext context, BufferAllocator allocator,
ChainedHashTable baseHashTable,
RecordBatch buildBatch, RecordBatch probeBatch,
boolean semiJoin,
int recordsPerBatch, SpillSet spillSet, int partNum,
int cycleNum, int numPartitions , boolean **isException** )
} catch (OutOfMemoryException oom) {
//do not call close ,do not throw except
isException =true;
}
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions,**isException** );
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
### 2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
but if
```
//add isException parameter when construct HashPartition object
HashPartition(FragmentContext context, BufferAllocator allocator,
ChainedHashTable baseHashTable,
RecordBatch buildBatch, RecordBatch probeBatch,
boolean semiJoin,
int recordsPerBatch, SpillSet spillSet, int partNum,
int cycleNum, int numPartitions , boolean **isException** )
} catch (OutOfMemoryException oom) {
//do not call close ,do not throw except
isException =true;
}
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions,**isException** );
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
### 2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
but if
```
HashPartition(FragmentContext context, BufferAllocator allocator,
ChainedHashTable baseHashTable,
RecordBatch buildBatch, RecordBatch probeBatch,
boolean semiJoin,
int recordsPerBatch, SpillSet spillSet, int partNum,
int cycleNum, int numPartitions , boolean **isException** )
} catch (OutOfMemoryException oom) {
//do not call close ,do not throw except
isException =false;
}
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions,**isException** );
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
### 2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
but if
```
} catch (OutOfMemoryException oom) {
//do not call close ,only throw except
throw UserException.memoryError(oom)
.message("Failed to allocate hash partition.")
.build(logger);
}
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1464211148
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
### 1. fix idea
The design is any operator fails, the entire operator stack is closed. but
partitions is array which is initialed by null。if hashPartition object is not
created successfully, it throw exception. so partitions array data after index
which is null。
` for (int part = 0; part < numPartitions; part++) {
partitions[part] = new HashPartition(context, allocator, baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}`
for example
partitions array length is 32, numPartitions =32 when numPartitions =10
,throw except. partitions[11-31] will be null
object which index numPartitions =10 was created failed ,but it had
allocater memory.
when calling close() , hashpartion object which numPartitions =10 can not
call close,beacause it is null。
2. another fix idea
we do not throw exception and do not call close, but catch. we can
create hash partiotn object . thus when calling close() , we can release。
but if
```
} catch (OutOfMemoryException oom) {
//do not call close ,only throw except
throw UserException.memoryError(oom)
.message("Failed to allocate hash partition.")
.build(logger);
}
AbstractHashBinaryRecordBatch#initializeBuild
boolean isException = false;
try {
for (int part = 0; part < numPartitions; part++) {
if (isException) {
break;
}
partitions[part] = new HashPartition(context, allocator,
baseHashTable,
buildBatch, probeBatch, semiJoin, RECORDS_PER_BATCH, spillSet,
part,
spilledState.getCycle(), numPartitions);
}
} catch (Exception e) {
isException = true;
}
if (isException ){
throw UserException.memoryError(exceptions[0])
.message("Failed to allocate hash partition.")
.build(logger);
}
```
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
paul-rogers commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1463921977
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/AbstractHashBinaryRecordBatch.java:
##
@@ -1312,7 +1312,9 @@ private void cleanup() {
}
// clean (and deallocate) each partition, and delete its spill file
for (HashPartition partn : partitions) {
- partn.close();
+ if (partn != null) {
+partn.close();
+ }
Review Comment:
The above is OK as a work-around. I wonder, however, where the code added a
null pointer to the partition list. That should never happen. If it does, it
should be fixed at the point where the null pointer is added to the list.
Fixing it here is incomplete: there are other places where we loop through the
list, and those will also fail.
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
Review Comment:
This call is _probably_ fine. However, the design is that if any operator
fails, the entire operator stack is closed. So, `close()` should be called by
the fragment executor. There is probably no harm in calling `close()` here, as
long as the `close()` method is safe to call twice.
If the fragment executor _does not_ call close when the failure occurs
during setup, then there is a bug since failing to call `close()` results in
just this kind of error.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
jnturton merged PR #2875: URL: https://github.com/apache/drill/pull/2875 -- 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. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on PR #2875: URL: https://github.com/apache/drill/pull/2875#issuecomment-1905599592 > [An unsued import crept in](https://github.com/apache/drill/actions/runs/7622586264/job/20762475705#step:6:1277), could you remove it please? removed it -- 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. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
jnturton commented on PR #2875: URL: https://github.com/apache/drill/pull/2875#issuecomment-1905598192 [An unsued import crept in](https://github.com/apache/drill/actions/runs/7622586264/job/20762475705#step:6:1277), could you remove it 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. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
shfshihuafeng commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1462854154
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
+ throw UserException.memoryError(oom)
+ .message("OutOfMemory while allocate memory for hash partition.")
Review Comment:
i resubmit pr and supply test step
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
paul-rogers commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1462817821
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
+ throw UserException.memoryError(oom)
+ .message("OutOfMemory while allocate memory for hash partition.")
Review Comment:
Suggested: `"Failed to allocate hash partition."`
The `memoryError()` already indicates that it is an OOM error.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8478. HashPartition memory leak when it allocate memory exception with OutOfMemoryException (#2874) (drill)
paul-rogers commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1462817821
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
-}
-this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
-tmpBatchesList = new ArrayList<>();
-if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+} catch (OutOfMemoryException oom) {
+ close();
+ throw UserException.memoryError(oom)
+ .message("OutOfMemory while allocate memory for hash partition.")
Review Comment:
Suggested: `"Failed to allocate hash partition."`
The `memoryError()` already indicate it is an OOM error.
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/AbstractHashBinaryRecordBatch.java:
##
@@ -1312,7 +1313,9 @@ private void cleanup() {
}
// clean (and deallocate) each partition, and delete its spill file
for (HashPartition partn : partitions) {
- partn.close();
+ if (Objects.nonNull(partn)) {
Review Comment:
Simpler `if (partn != null) {`
--
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.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
