You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by adeneche <gi...@git.apache.org> on 2015/09/21 21:23:39 UTC
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
GitHub user adeneche opened a pull request:
https://github.com/apache/drill/pull/163
DRILL-3811: AtomicRemainder incorrectly accounts for transferred allo…
…cations
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/adeneche/incubator-drill fix-acountor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/163.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #163
----
commit 94af90b8006ea95fdfa02cc4ddae5224ab31c586
Author: adeneche <ad...@gmail.com>
Date: 2015-09-20T19:11:55Z
DRILL-3811: AtomicRemainder incorrectly accounts for transferred allocations
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by cwestin <gi...@git.apache.org>.
Github user cwestin commented on the pull request:
https://github.com/apache/drill/pull/163#issuecomment-142102955
Just one comment, otherwise +1 (non-binding).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/163#issuecomment-142120660
TopLevelAllocator's constructor is not public but because the unit test is in the same package it was able to access the constructor anyway. Fixed the test to use RootAllocatorFactory instead
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by cwestin <gi...@git.apache.org>.
Github user cwestin commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40023098
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java ---
@@ -63,6 +63,29 @@
private final static String planFile="/physical_allocator_test.json";
@Test
+ public void testTransfer() throws Exception {
+ final Properties props = new Properties() {
+ {
+ put(ExecConstants.TOP_LEVEL_MAX_ALLOC, "1000000");
+ put(ExecConstants.ERROR_ON_MEMORY_LEAK, "true");
+ }
+ };
+ final DrillConfig config = DrillConfig.create(props);
+ BufferAllocator a = new TopLevelAllocator(config);
--- End diff --
I was surprised this compiled; in one of my recent patches, I had removed the "public" from TopLevelAllocator, but it seems to have come back. It was removed to force people to use RootAllocatorFactory.newRoot() instead of using the TopLevelAllocator constructor directly. Can you please remove the public from TopLevelAllocator again, and replace this constructor here and on the next line with RootAllocatorFactory.newRoot()?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/163
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40024717
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java ---
@@ -63,6 +63,29 @@
private final static String planFile="/physical_allocator_test.json";
@Test
+ public void testTransfer() throws Exception {
+ final Properties props = new Properties() {
+ {
+ put(ExecConstants.TOP_LEVEL_MAX_ALLOC, "1000000");
+ put(ExecConstants.ERROR_ON_MEMORY_LEAK, "true");
+ }
+ };
+ final DrillConfig config = DrillConfig.create(props);
+ BufferAllocator a = new TopLevelAllocator(config);
--- End diff --
sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/163#issuecomment-142170314
+1 .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40045182
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java ---
@@ -76,16 +74,17 @@ public void setLimit(long limit) {
}
/**
- * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested. This
- * memory has already been used up so it must be accurately accounted for in future allocations.
+ * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested, or when
+ * a buffer has it's ownership passed to another allocator.<br>
+ * This memory has already been used up so it must be accurately accounted for in future allocations.
*
- * @param size
+ * @param size extra allocated memory that needs to be accounted for
*/
public boolean forceGet(long size) {
if (get(size, this.applyFragmentLimit)) {
return true;
} else {
- availableShared.addAndGet(size);
+ availableShared.addAndGet(-size);
--- End diff --
Since this will be subtracting size bytes, should there be a check for availableShared >=0 ? I am not quite sure what's supposed to happen if this value drops below 0 at this point...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by cwestin <gi...@git.apache.org>.
Github user cwestin commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40109454
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java ---
@@ -76,16 +74,17 @@ public void setLimit(long limit) {
}
/**
- * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested. This
- * memory has already been used up so it must be accurately accounted for in future allocations.
+ * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested, or when
+ * a buffer has it's ownership passed to another allocator.<br>
+ * This memory has already been used up so it must be accurately accounted for in future allocations.
*
- * @param size
+ * @param size extra allocated memory that needs to be accounted for
*/
public boolean forceGet(long size) {
if (get(size, this.applyFragmentLimit)) {
return true;
} else {
- availableShared.addAndGet(size);
+ availableShared.addAndGet(-size);
--- End diff --
This call is used when transferring ownership from the RPC layer to a receiving fragment -- and you can't refuse it -- the RPC layer has to hand it off. The problem is that the accounting for buffer ownership transfer and sharing is questionable in the original allocator. This is probably the best that can be done for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40047181
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java ---
@@ -76,16 +74,17 @@ public void setLimit(long limit) {
}
/**
- * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested. This
- * memory has already been used up so it must be accurately accounted for in future allocations.
+ * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested, or when
+ * a buffer has it's ownership passed to another allocator.<br>
+ * This memory has already been used up so it must be accurately accounted for in future allocations.
*
- * @param size
+ * @param size extra allocated memory that needs to be accounted for
*/
public boolean forceGet(long size) {
if (get(size, this.applyFragmentLimit)) {
return true;
} else {
- availableShared.addAndGet(size);
+ availableShared.addAndGet(-size);
--- End diff --
It can become negative if the allocator takes ownership of a buffer and exceeds it's maximum allocated memory. Negative values are handled properly, at least that's my understanding
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/163#discussion_r40050662
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java ---
@@ -76,16 +74,17 @@ public void setLimit(long limit) {
}
/**
- * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested. This
- * memory has already been used up so it must be accurately accounted for in future allocations.
+ * Automatically allocate memory. This is used when an actual allocation happened to be larger than requested, or when
+ * a buffer has it's ownership passed to another allocator.<br>
+ * This memory has already been used up so it must be accurately accounted for in future allocations.
*
- * @param size
+ * @param size extra allocated memory that needs to be accounted for
*/
public boolean forceGet(long size) {
if (get(size, this.applyFragmentLimit)) {
return true;
} else {
- availableShared.addAndGet(size);
+ availableShared.addAndGet(-size);
--- End diff --
It is likely handled..I will defer to Chris on that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---