You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by jacques-n <gi...@git.apache.org> on 2016/01/16 01:09:55 UTC
[GitHub] drill pull request: DRILL-4131: Move RPC allocators under Drill's ...
GitHub user jacques-n opened a pull request:
https://github.com/apache/drill/pull/327
DRILL-4131: Move RPC allocators under Drill's root allocator & accounting
- Allow settings to be set to ensure RPC reservation and maximums (currently unset by default). Defaults set in drill-module.conf
- Add new metrics to report RPC layer memory consumption.
- Check for memory leaks from RPC layer at shutdown.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacques-n/drill DRILL-4131
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/327.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 #327
----
commit daa89a68aa416f8718ad005bc57baf4ac58a9c66
Author: Jacques Nadeau <ja...@apache.org>
Date: 2016-01-15T23:53:13Z
DRILL-4131: Move RPC allocators under Drill's root allocator & accounting
- Allow settings to be set to ensure RPC reservation and maximums (currently unset by default). Defaults set in drill-module.conf
- Add new metrics to report RPC layer memory consumption.
- Check for memory leaks from RPC layer at shutdown.
----
---
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-4131: Move RPC allocators under Drill's ...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50033210
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java ---
@@ -56,22 +60,83 @@
private final DrillConfig config;
boolean useIP = false;
private final boolean allowPortHunting;
+ private final BufferAllocator userAllocator;
+ private final BufferAllocator controlAllocator;
+ private final BufferAllocator dataAllocator;
+
public ServiceEngine(ControlMessageHandler controlMessageHandler, UserWorker userWorker, BootStrapContext context,
WorkEventBus workBus, WorkerBee bee, boolean allowPortHunting) throws DrillbitStartupException {
+ userAllocator = newAllocator(context, "rpc:user", "drill.exec.rpc.user.server.memory.reservation",
+ "drill.exec.rpc.user.server.memory.maximum");
+ controlAllocator = newAllocator(context, "rpc:bit-control",
+ "drill.exec.rpc.bit.server.memory.control.reservation", "drill.exec.rpc.bit.server.memory.control.maximum");
+ dataAllocator = newAllocator(context, "rpc:bit-control",
--- End diff --
nice catch
---
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-4131: Move RPC allocators under Drill's ...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/327#issuecomment-172623959
One quick note, I still am receiving failures on a small set of unit tests because of metric registration conflicts so I'll be fixing those. (Happens only in tests where a single JVM contains multiple drillbits since the registry is currently static.) Also, I'm concerned that some portion of allocation is not getting tracked since the tests that Vicky just posted on DRILL-4266 don't show a zero peak allocation for the RPC data layer (which doesn't make any sense unless all the queries were single fragmented).
---
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-4131: Move RPC allocators under Drill's ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50024752
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java ---
@@ -56,22 +60,83 @@
private final DrillConfig config;
boolean useIP = false;
private final boolean allowPortHunting;
+ private final BufferAllocator userAllocator;
+ private final BufferAllocator controlAllocator;
+ private final BufferAllocator dataAllocator;
+
public ServiceEngine(ControlMessageHandler controlMessageHandler, UserWorker userWorker, BootStrapContext context,
WorkEventBus workBus, WorkerBee bee, boolean allowPortHunting) throws DrillbitStartupException {
+ userAllocator = newAllocator(context, "rpc:user", "drill.exec.rpc.user.server.memory.reservation",
+ "drill.exec.rpc.user.server.memory.maximum");
+ controlAllocator = newAllocator(context, "rpc:bit-control",
+ "drill.exec.rpc.bit.server.memory.control.reservation", "drill.exec.rpc.bit.server.memory.control.maximum");
+ dataAllocator = newAllocator(context, "rpc:bit-control",
--- End diff --
should it be named `rpc:bit-data` ?
---
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-4131: Move RPC allocators under Drill's ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50026812
--- Diff: exec/memory/base/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java ---
@@ -59,7 +59,7 @@ public PooledByteBufAllocatorL(MetricRegistry registry) {
public UnsafeDirectLittleEndian allocate(int size) {
try {
- return allocator.directBuffer(size, size);
+ return allocator.directBuffer(size, Integer.MAX_VALUE);
--- End diff --
Out of curiosity, why this change ?
---
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-4131: Move RPC allocators under Drill's ...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50182483
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java ---
@@ -56,22 +60,83 @@
private final DrillConfig config;
boolean useIP = false;
private final boolean allowPortHunting;
+ private final BufferAllocator userAllocator;
+ private final BufferAllocator controlAllocator;
+ private final BufferAllocator dataAllocator;
+
public ServiceEngine(ControlMessageHandler controlMessageHandler, UserWorker userWorker, BootStrapContext context,
WorkEventBus workBus, WorkerBee bee, boolean allowPortHunting) throws DrillbitStartupException {
+ userAllocator = newAllocator(context, "rpc:user", "drill.exec.rpc.user.server.memory.reservation",
+ "drill.exec.rpc.user.server.memory.maximum");
+ controlAllocator = newAllocator(context, "rpc:bit-control",
+ "drill.exec.rpc.bit.server.memory.control.reservation", "drill.exec.rpc.bit.server.memory.control.maximum");
+ dataAllocator = newAllocator(context, "rpc:bit-control",
--- End diff --
Someone argued recently that it doesn't make sense to put everything there and I am in agreement. I've started adopting keeping strings where they are relevant.
---
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-4131: Move RPC allocators under Drill's ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/327
---
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-4131: Move RPC allocators under Drill's ...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50033277
--- Diff: exec/memory/base/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java ---
@@ -59,7 +59,7 @@ public PooledByteBufAllocatorL(MetricRegistry registry) {
public UnsafeDirectLittleEndian allocate(int size) {
try {
- return allocator.directBuffer(size, size);
+ return allocator.directBuffer(size, Integer.MAX_VALUE);
--- End diff --
The RPC uses ensureWritable to expand buffers. Buffers are expandable to maxCapacity. We were coding maxCapacity == capacity which meant that buffers weren't expandable and the RPC layer would error out. This fixes this issue.
---
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-4131: Move RPC allocators under Drill's ...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/327#issuecomment-172604924
apart from a small code change, LGTM.
Let's run a couple of tests (performance and concurrency) before merging this PR.
+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-4131: Move RPC allocators under Drill's ...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/327#discussion_r50167961
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java ---
@@ -56,22 +60,83 @@
private final DrillConfig config;
boolean useIP = false;
private final boolean allowPortHunting;
+ private final BufferAllocator userAllocator;
+ private final BufferAllocator controlAllocator;
+ private final BufferAllocator dataAllocator;
+
public ServiceEngine(ControlMessageHandler controlMessageHandler, UserWorker userWorker, BootStrapContext context,
WorkEventBus workBus, WorkerBee bee, boolean allowPortHunting) throws DrillbitStartupException {
+ userAllocator = newAllocator(context, "rpc:user", "drill.exec.rpc.user.server.memory.reservation",
+ "drill.exec.rpc.user.server.memory.maximum");
+ controlAllocator = newAllocator(context, "rpc:bit-control",
+ "drill.exec.rpc.bit.server.memory.control.reservation", "drill.exec.rpc.bit.server.memory.control.maximum");
+ dataAllocator = newAllocator(context, "rpc:bit-control",
--- End diff --
Can you please move the reservation and maximum string constants to ExecConstants?
---
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.
---