[GitHub] drill pull request #1190: DRILL-5937: ExecConstants: changed comment, timeou...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1190#discussion_r177300882
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -558,7 +558,7 @@ private ExecConstants() {
/**
* Timeout for create prepare statement request. If the request exceeds
this timeout, then request is timed out.
- * Default value is 10mins.
+ * Default value is 30 seconds.
--- End diff --
I refer to the entire comment. "If the request exceeds this timeout, then
request is timed out" does not add any value, it is simply a definition for a
timeout. The same for `CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS` and "Timeout
for create prepare statement request".
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1189 @vdiravka right, check the first link, the new `groupId` for `com.codahale.metrics` is `io.dropwizard.metrics`, so all new versions will be deployed using the new `groupId`. ---
[GitHub] drill pull request #1185: DRILL-6288: Upgrade org.javassist:javassist and or...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1185#discussion_r177499603 --- Diff: exec/jdbc-all/pom.xml --- @@ -559,7 +559,7 @@ This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users. -3200 +3300 --- End diff -- Yes. ---
[GitHub] drill issue #1182: DRILL-6287: apache-release profile should be disabled by ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1182 @parthchandra Please review ---
[GitHub] drill pull request #1190: DRILL-5937: ExecConstants: changed comment, timeou...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1190#discussion_r177103076
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -558,7 +558,7 @@ private ExecConstants() {
/**
* Timeout for create prepare statement request. If the request exceeds
this timeout, then request is timed out.
- * Default value is 10mins.
+ * Default value is 30 seconds.
--- End diff --
Is it 30 seconds or 10 seconds? In any case, I don't think that the entire
comment provides any additional info that cannot be inferred from the code
itself.
---
[GitHub] drill issue #1182: DRILL-6287: apache-release profile should be disabled by ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1182 There are two issues with enabling `apache-release` by default: - it triggers creating source `apache-drill-...-src.tar.gz` and `apache-drill-...-src.zip` archives. - maven build for any sub-module fails. The change disables activation of the `apache-release` profile based on JDK version and requires explicit activation during the Apache release process. JDK 1.7 is not supported. See DRILL-1491 and #1143. ---
[GitHub] drill pull request #1182: DRILL-6287: apache-release profile should be disab...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1182#discussion_r178567909 --- Diff: pom.xml --- @@ -66,6 +66,7 @@ 4096 4096 +-Xdoclint:none --- End diff -- @vdiravka Please see DRILL-4547. ---
[GitHub] drill issue #1144: DRILL-6202: Deprecate usage of IndexOutOfBoundsException ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1144 It is not clear why get/set Byte/Char/Short/Int/Long/Float/Double do not delegate to UDLE, while get/set Bytes delegates to UDLE and relies on netty 'AbstractByteBuf` for bounds checking. IMO, it will be good to have the behavior consistent for all methods. In many cases including `VariableLengthVectors`, there is no need to rely on UDLE boundary checking as a caller already provides or can provide a guarantee that an index is within a buffer boundaries. In those cases, boundary check becomes an extra cost. IMO, it will be good to have a consistent behavior with ability to enable bounds checking for debugging. ---
[GitHub] drill issue #1144: DRILL-6202: Deprecate usage of IndexOutOfBoundsException ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1144 IMO, this PR or DRILL-6202 is not the best place to discuss boundary checking as the PR and JIRA deals with `IndexOutOfBoundsException` but does not change how DrillBuf, Vector or Operators ensure that reads and writes are done within proper boundaries. I'll bring that to dev@drill. Please repeat your arguments on the mailing list. ---
[GitHub] drill pull request #1195: DRILL-6273: Removed dependency licensed under Cate...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1195#discussion_r178953868 --- Diff: tools/fmpp/src/main/java/bsh/package-info.java --- @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + /** + * Generate-fmpp has a dependency on beanshell EvalError. Beanshell doesn't have a valid + * Apache License, So beanshell is excluded and EvalError class is added to handle the dependency. + */ + +package bsh; --- End diff -- Please add LF and squash commits. ---
[GitHub] drill pull request #1195: DRILL-6273: Removed dependency licensed under Cate...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1195#discussion_r178364790 --- Diff: tools/fmpp/src/main/java/bsh/EvalError.java --- @@ -0,0 +1,28 @@ +/** --- End diff -- Please do not use doc comment for the license. ---
[GitHub] drill pull request #1195: DRILL-6273: Removed dependency licensed under Cate...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1195#discussion_r178365989 --- Diff: tools/fmpp/src/main/java/bsh/EvalError.java --- @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package bsh; +/** --- End diff -- will be better to have this comment in package-info. ---
[GitHub] drill pull request #1195: DRILL-6273: Removed dependency licensed under Cate...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1195#discussion_r178366218 --- Diff: tools/fmpp/pom.xml --- @@ -57,6 +57,10 @@ commons-logging-api commons-logging + + bsh --- End diff -- add bsh:org.beanshell to the prohibited dependencies. ---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1196#discussion_r178376362
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java ---
@@ -212,19 +218,29 @@ private boolean areQueriesAndFragmentsEmpty() {
return queries.isEmpty() && runningFragments.isEmpty();
}
+ /**
+ * Check if there any new queries or fragments that are added after the
shutdown is triggered
+ */
+ private boolean areNewQueriesOrFragmentsAdded() {
+return runningFragments.size() > numOfRunningFragments ||
queries.size() > numOfRunningQueries;
+ }
+
/**
* A thread calling the {@link #waitToExit(boolean)} method is notified
when a foreman is retired.
*/
private void indicateIfSafeToExit() {
isEmptyLock.lock();
try {
- logger.info("Waiting for "+ queries.size() +" queries to complete
before shutting down");
- logger.info("Waiting for "+ runningFragments.size() +" running
fragments to complete before shutting down");
+ if (isShutdownTriggered) {
+logger.info("Waiting for "+ queries.size() +" queries to complete
before shutting down");
--- End diff --
Use slf4j smart logging.
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1196#discussion_r178376213
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java ---
@@ -212,19 +218,29 @@ private boolean areQueriesAndFragmentsEmpty() {
return queries.isEmpty() && runningFragments.isEmpty();
}
+ /**
+ * Check if there any new queries or fragments that are added after the
shutdown is triggered
+ */
+ private boolean areNewQueriesOrFragmentsAdded() {
+return runningFragments.size() > numOfRunningFragments ||
queries.size() > numOfRunningQueries;
--- End diff --
This condition is not reliable. What if some fragments exited and some were
added? The total number may still be less than the number of fragments when the
shutdown was requested.
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1196#discussion_r178375583 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java --- @@ -86,6 +86,9 @@ private final StatusThread statusThread; private final Lock isEmptyLock = new ReentrantLock(); private final Condition isEmptyCondition = isEmptyLock.newCondition(); + private boolean isShutdownTriggered = false; --- End diff -- Is this boolean necessary? Can you delay getting `isEmptyCondition` till shutdown is requested and use it in place of `isShutdownTriggered`? ---
[GitHub] drill issue #1105: DRILL-6125: Fix possible memory leak when query is finish...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1105 @arina-ielchiieva Please review. ---
[GitHub] drill issue #1144: DRILL-6202: Deprecate usage of IndexOutOfBoundsException ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1144 Is there a reason to delegate `get/setBytes` to `AbstractByteBuf`? If not, this PR will be a preparation step to use `PlatformDependent` directly bypassing Netty bounds checking. ---
[GitHub] drill pull request #1182: DRILL-6287: apache-release profile should be disab...
GitHub user vrozov opened a pull request: https://github.com/apache/drill/pull/1182 DRILL-6287: apache-release profile should be disabled by default @parthchandra @dvjyothsna Please review You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrozov/drill DRILL-6287 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1182.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 #1182 commit 23745ab83305373f5d8040ed65070d82b5ea4aa7 Author: Vlad Rozov <vrozov@...> Date: 2018-03-22T13:54:56Z DRILL-6287: apache-release profile should be disabled by default ---
[GitHub] drill pull request #1177: DRILL-6280: Cleanup execution of BuildTimeScan dur...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1177#discussion_r176260351
--- Diff: exec/java-exec/pom.xml ---
@@ -828,31 +828,9 @@
-
+
org.codehaus.mojo
exec-maven-plugin
-1.2.1
-
-
-org.apache.drill
-drill-common
-${project.version}
-tests
-
-
-
-
-process-classes
-java
-
-
-
-
org.apache.drill.common.scanner.BuildTimeScan
- true
--- End diff --
The plugin is now configured in the `PluginManagement` section in the drill
root pom.
---
[GitHub] drill issue #1177: DRILL-6280: Cleanup execution of BuildTimeScan during mav...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1177 @vdiravka Please clarify your concern regarding logback-classic. Without the fix, logback-classic is not on the classpath during `exec:java` maven plugin execution. This causes the following issue: ``` [INFO] --- exec-maven-plugin:1.2.1:java (default) @ drill-common --- SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details. ``` ---
[GitHub] drill pull request #1177: DRILL-6280: Cleanup execution of BuildTimeScan dur...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1177#discussion_r176262322
--- Diff: common/pom.xml ---
@@ -113,20 +113,10 @@
org.codehaus.mojo
exec-maven-plugin
-1.2.1
-
-
-process-classes
-
- java
-
-
-
-
org.apache.drill.common.scanner.BuildTimeScan
-
-${project.build.outputDirectory}
-
+
+
${project.basedir}/src/test/resources/
--- End diff --
The `logback-test.xml` was not in the classpath before, but it did not
matter as slf4j did not bind to logback classic before anyway (it was not on
classpath).
---
[GitHub] drill issue #1163: DRILL-6053 & DRILL-6237
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1163 @arina-ielchiieva Addressed review comments. Please keep both commits (do not squash) during the merge. ---
[GitHub] drill pull request #1177: DRILL-6280: Cleanup execution of BuildTimeScan dur...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1177#discussion_r176263130 --- Diff: exec/java-exec/pom.xml --- @@ -828,31 +828,9 @@ - + org.codehaus.mojo exec-maven-plugin -1.2.1 - - -org.apache.drill -drill-common --- End diff -- `drill-common` and `drill-common-test` are included as part of `test` scope dependencies ---
[GitHub] drill pull request #1185: DRILL-6288: Upgrade org.javassist:javassist and or...
GitHub user vrozov opened a pull request: https://github.com/apache/drill/pull/1185 DRILL-6288: Upgrade org.javassist:javassist and org.reflections:reflections @vdiravka Please review You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrozov/drill DRILL-6288 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1185.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 #1185 commit d6b74d59af0cb8fa39b3deb73846a3b45acddfba Author: Vlad Rozov <vrozov@...> Date: 2018-03-22T22:47:31Z DRILL-6288: Upgrade org.javassist:javassist and org.reflections:reflections ---
[GitHub] drill pull request #1177: DRILL-6280: Cleanup execution of BuildTimeScan dur...
GitHub user vrozov opened a pull request: https://github.com/apache/drill/pull/1177 DRILL-6280: Cleanup execution of BuildTimeScan during maven build You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrozov/drill DRILL-6280 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1177.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 #1177 commit 5305320bb3745102dae071a035648f377de84464 Author: Vlad Rozov <vrozov@...> Date: 2018-03-21T02:24:11Z DRILL-6280: Cleanup execution of BuildTimeScan during maven build ---
[GitHub] drill issue #1177: DRILL-6280: Cleanup execution of BuildTimeScan during mav...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1177 @vdiravka Please review ---
[GitHub] drill pull request #1144: DRILL-6202: Deprecate usage of IndexOutOfBoundsExc...
GitHub user vrozov opened a pull request: https://github.com/apache/drill/pull/1144 DRILL-6202: Deprecate usage of IndexOutOfBoundsException to re-alloc vectors You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrozov/drill DRILL-6202 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1144.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 #1144 commit 2af94a07340f9f13aa152822c2c8d37568ab44ab Author: Vlad Rozov <vrozov@...> Date: 2018-03-01T17:36:05Z DRILL-6202: Deprecate usage of IndexOutOfBoundsException to re-alloc vectors ---
[GitHub] drill issue #1144: DRILL-6202: Deprecate usage of IndexOutOfBoundsException ...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1144 @parthchandra Please take a look. ---
[GitHub] drill issue #1105: DRILL-6125: Fix possible memory leak when query is cancel...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1105 @ilooner What state is protected by `syncrhonized`? Why is it not sufficient to use `volatile` and `AtomicReference`? ---
[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...
GitHub user vrozov opened a pull request: https://github.com/apache/drill/pull/1148 DRILL-5994: enable configuring number of Jetty acceptors and selectors (default to 1 acceptor and 2 selectors) @arina-ielchiieva @ilooner @MitchelLabonte Please review You can merge this pull request into a Git repository by running: $ git pull https://github.com/vrozov/drill DRILL-5994 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1148.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 #1148 commit d4feca33e71218905ac5ea1aad55c0b77af2c305 Author: Vlad Rozov <vrozov@...> Date: 2018-03-02T18:39:31Z DRILL-5994: enable configuring number of Jetty acceptors and selectors (default to 1 acceptor and 2 selectors) ---
[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1148#discussion_r171987801
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -154,35 +152,31 @@ public void start() throws Exception {
final boolean authEnabled =
config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
-port = config.getInt(ExecConstants.HTTP_PORT);
-boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
-int retry = 0;
-
-for (; retry < PORT_HUNT_TRIES; retry++) {
- embeddedJetty = new Server(new
QueuedThreadPool(config.getInt(ExecConstants.WEB_SERVER_THREAD_POOL_MAX)));
- embeddedJetty.setHandler(createServletContextHandler(authEnabled));
- embeddedJetty.addConnector(createConnector(port));
-
+int port = config.getInt(ExecConstants.HTTP_PORT);
+final boolean portHunt =
config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+final int acceptors =
config.getInt(ExecConstants.HTTP_JETTY_SERVER_ACCEPTORS);
+final int selectors =
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
+final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
+embeddedJetty = new Server(threadPool);
+embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServerConnector connector = createConnector(port, acceptors,
selectors);
+threadPool.setMaxThreads(1 + connector.getAcceptors() +
connector.getSelectorManager().getSelectorCount());
+embeddedJetty.addConnector(connector);
+for (int retry = 0; retry < PORT_HUNT_TRIES; retry++) {
+ connector.setPort(port);
try {
embeddedJetty.start();
+return;
} catch (BindException e) {
if (portHunt) {
- int nextPort = port + 1;
- logger.info("Failed to start on port {}, trying port {}", port,
nextPort);
- port = nextPort;
- embeddedJetty.stop();
+ logger.info("Failed to start on port {}, trying port {}", port,
++port, e);
--- End diff --
The QueuedThreadPool is now provided and will be reused. Please see line
159.
---
[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1148#discussion_r171989096
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -424,10 +418,11 @@ private ServerConnector createHttpsConnector(int
port) throws Exception {
* @return Initialized {@link ServerConnector} instance for HTTP
connections.
* @throws Exception
*/
- private ServerConnector createHttpConnector(int port) throws Exception {
+ private ServerConnector createHttpConnector(int port, int acceptors, int
selectors) throws Exception {
logger.info("Setting up HTTP connector for web server");
final HttpConfiguration httpConfig = new HttpConfiguration();
-final ServerConnector httpConnector = new
ServerConnector(embeddedJetty, new HttpConnectionFactory(httpConfig));
+final ServerConnector httpConnector =
+new ServerConnector(embeddedJetty, null, null, null, acceptors,
selectors, new HttpConnectionFactory(httpConfig));
--- End diff --
I don't think there is such constructor (only one that takes
`SslContextFactory` as the last argument), please double check.
---
[GitHub] drill issue #755: DRILL-5270: Improve loading of profiles listing in the Web...
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/755 @kkhatua 1. The read locks are not exclusive (single writer/multiple readers). To achieve the required functionality you need to introduce a different lock and use write (or exclusive) lock. 2. The choice for TreeSet is not obvious. What are the most common operations performed on the collection? Do you optimize for get, put or collection construction? @arina-ielchiieva my github id is `vrozov`. ---
[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1148#discussion_r171988826
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -411,6 +404,7 @@ private ServerConnector createHttpsConnector(int port)
throws Exception {
// SSL Connector
final ServerConnector sslConnector = new ServerConnector(embeddedJetty,
+null, null, null, -1, -1,
--- End diff --
good catch ð
---
[GitHub] drill pull request #1105: DRILL-6125: Fix possible memory leak when query is...
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1105#discussion_r170782805 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java --- @@ -62,7 +62,7 @@ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(PartitionSenderRootExec.class); private RecordBatch incoming; private HashPartitionSender operator; - private PartitionerDecorator partitioner; + private volatile PartitionerDecorator partitioner; --- End diff -- Consider using `AtomicReference` instead of `volatile` ---
[GitHub] drill pull request #1237: DRILL-6348: Fixed code so that Unordered Receiver ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1237#discussion_r184154997
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/record/RawFragmentBatch.java
---
@@ -77,4 +83,46 @@ public long getByteCount() {
public boolean isAckSent() {
return ackSent.get();
}
+
+ /**
+ * Transfer ownership of this DrillBuf to the target allocator. This is
done for better memory
+ * accounting (that is, the operator should be charged with the body's
Drillbuf memory).
+ *
+ * NOTES -
+ *
+ * This operation is a NOOP when a) the current allocator
(associated with the DrillBuf) is not the
+ * owning allocator or b) the target allocator is already the owner
+ * When transfer happens, a new RawFragmentBatch instance is
allocated; this is done for proper
+ * DrillBuf reference count accounting
+ * The RPC handling code caches a reference to this RawFragmentBatch
object instance; release()
+ * calls should be routed to the previous DrillBuf
+ *
+ *
+ * @param targetAllocator target allocator
+ * @return a new {@link RawFragmentBatch} object instance on success
(where the buffer ownership has
+ * been switched to the target allocator); otherwise this
operation is a NOOP (current instance
+ * returned)
+ */
+ public RawFragmentBatch transferBodyOwnership(BufferAllocator
targetAllocator) {
+if (body == null) {
+ return this; // NOOP
+}
+
+if (!body.getLedger().isOwningLedger()
+ || body.getLedger().isOwner(targetAllocator)) {
+
+ return this;
+}
+
+int writerIndex = body.writerIndex();
+TransferResult transferResult =
body.transferOwnership(targetAllocator);
+
+// Set the index and increment reference count
+transferResult.buffer.writerIndex(writerIndex);
+
+// Clear the current Drillbuffer since caller will perform release()
on the new one
+body.release();
+
+return new RawFragmentBatch(getHeader(), transferResult.buffer,
getSender(), false);
--- End diff --
I don't see where RawFragmentBatch is cached. Is not it removed from a
queue using poll()?
---
[GitHub] drill pull request #1237: DRILL-6348: Fixed code so that Unordered Receiver ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1237#discussion_r184156922
--- Diff:
exec/memory/base/src/main/java/org/apache/drill/exec/memory/AllocationManager.java
---
@@ -253,10 +261,12 @@ public boolean transferBalance(final BufferLedger
target) {
target.historicalLog.recordEvent("incoming(from %s)",
owningLedger.allocator.name);
}
-boolean overlimit = target.allocator.forceAllocate(size);
+// Release first to handle the case where the current and target
allocators were part of the same
+// parent / child tree.
allocator.releaseBytes(size);
+boolean allocationFit = target.allocator.forceAllocate(size);
--- End diff --
If this happens, is not there a problem that the old allocator already
released the memory? In any case, won't runtime exception cancel the query
anyway and all allocators will be closed.
---
[GitHub] drill pull request #1237: DRILL-6348: Fixed code so that Unordered Receiver ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1237#discussion_r184159218
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java
---
@@ -153,8 +153,10 @@ private RawFragmentBatch getNextBatch() throws
IOException {
public IterOutcome next() {
batchLoader.resetRecordCount();
stats.startProcessing();
+
+RawFragmentBatch batch = null;
try{
- RawFragmentBatch batch;
+
--- End diff --
create function `getNextNotEmptyBatch()` that calls `getNextBatch()` and
either returns not empty batch or `null`.
---
[GitHub] drill pull request #1237: DRILL-6348: Fixed code so that Unordered Receiver ...
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1237#discussion_r184155724
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/record/RawFragmentBatch.java
---
@@ -77,4 +83,46 @@ public long getByteCount() {
public boolean isAckSent() {
return ackSent.get();
}
+
+ /**
+ * Transfer ownership of this DrillBuf to the target allocator. This is
done for better memory
+ * accounting (that is, the operator should be charged with the body's
Drillbuf memory).
+ *
+ * NOTES -
+ *
+ * This operation is a NOOP when a) the current allocator
(associated with the DrillBuf) is not the
+ * owning allocator or b) the target allocator is already the owner
+ * When transfer happens, a new RawFragmentBatch instance is
allocated; this is done for proper
+ * DrillBuf reference count accounting
+ * The RPC handling code caches a reference to this RawFragmentBatch
object instance; release()
+ * calls should be routed to the previous DrillBuf
+ *
+ *
+ * @param targetAllocator target allocator
+ * @return a new {@link RawFragmentBatch} object instance on success
(where the buffer ownership has
+ * been switched to the target allocator); otherwise this
operation is a NOOP (current instance
+ * returned)
+ */
+ public RawFragmentBatch transferBodyOwnership(BufferAllocator
targetAllocator) {
+if (body == null) {
+ return this; // NOOP
+}
+
+if (!body.getLedger().isOwningLedger()
+ || body.getLedger().isOwner(targetAllocator)) {
+
+ return this;
+}
+
+int writerIndex = body.writerIndex();
+TransferResult transferResult =
body.transferOwnership(targetAllocator);
--- End diff --
But what if it is an over limit?
---
[GitHub] drill issue #1189: DRILL-6282: Update Drill's Metrics dependencies
Github user vrozov commented on the issue: https://github.com/apache/drill/pull/1189 LGTM ---
[GitHub] drill pull request #1189: DRILL-6282: Update Drill's Metrics dependencies
Github user vrozov commented on a diff in the pull request: https://github.com/apache/drill/pull/1189#discussion_r184243564 --- Diff: logical/pom.xml --- @@ -85,14 +85,12 @@ - com.codahale.metrics + io.dropwizard.metrics --- End diff -- It is not a best practice to rely on a transitive dependency for a compile scope dependency. I'd recommend specifying the dependency on `io.dropwizard.metrics` explicitly in case `java-exec` or `drill-memory-base` uses it. ---
