[GitHub] drill pull request: DRILL-4483: Fix text plan regression in query ...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/411#issuecomment-193531465 +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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4499: Remove 17 unused classes
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/426 DRILL-4499: Remove 17 unused classes You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4499 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/426.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 #426 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: Add Sudheesh to Drill Committers' List
Github user sudheeshkatkam closed the pull request at: https://github.com/apache/drill/pull/268 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: Drill 4504: Create an event loop for each of [...
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/429 Drill 4504: Create an event loop for each of [user, control, data] RPC components You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4504 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/429.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 #429 commit 83821303dc5adf3e5ac6f033c9f04ad10685f332 Author: Sudheesh Katkam Date: 2016-03-09T18:34:26Z DRILL-4504: HYGIENE + Merge DrillAutoCloseables and AuthCloseables + Move WorkEventBus from exec/rpc/control to exec/work + Remove unused imports commit 3bc39e71da9c6af47e2c5f1d67b03eea8aafa0d9 Author: Sudheesh Katkam Date: 2016-03-11T21:27:38Z DRILL-4504: CORE + Add DrillClient.Builder helper class to create DrillClient objects + Deprecate 9 constructors commit 68636983ff25049cf1d31f30516544812cabbc0e Author: Sudheesh Katkam Date: 2016-03-09T19:59:50Z DRILL-4504: CORE + Create an event loop group for each client-server pair [data, client and user] + Use user loop group when creating a DrillClient in web API calls + Shutdown event loop groups in BootstrapContext + Use MoreExecutors#shutdownAndAwaitTermination to shutdown the worker pool commit f138ad19658d0c4d8fbb7c782a82edab5ab2f142 Author: Sudheesh Katkam Date: 2016-03-12T00:57:31Z DRILL-4504: CLEANUP + Cleanup exec.rpc.control.* + Remove unused methods + Add missing Override annotations + Lower visibility of DefaultInstanceHandler and classes in ControlTunnel Documentation: + Rename methods and variables for better description + Add package-info.java with an overview of rpc.control package + Formatting changes (mainly spacing) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r56080621
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -74,73 +74,148 @@
/**
* Thin wrapper around a UserClient that handles connect/close and
transforms
* String into ByteBuf.
+ *
+ * Use the builder class ({@link DrillClient.Builder}) to build objects of
this class.
+ * E.g.
+ *
+ * DrillClient client = DrillClient.newBuilder()
+ * .setConfig(...)
+ * .setIsDirectConnection(true)
+ * .build();
+ *
*/
public class DrillClient implements Closeable, ConnectionThrottle {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillClient.class);
private final DrillConfig config;
- private UserClient client;
- private UserProperties props = null;
- private volatile ClusterCoordinator clusterCoordinator;
- private volatile boolean connected = false;
private final BufferAllocator allocator;
- private int reconnectTimes;
- private int reconnectDelay;
- private boolean supportComplexTypes;
- private final boolean ownsZkConnection;
+ private final boolean isDirectConnection;
+ private final int reconnectTimes;
+ private final int reconnectDelay;
+
+ // checks if this client owns these resources (used when closing)
private final boolean ownsAllocator;
- private final boolean isDirectConnection; // true if the connection
bypasses zookeeper and connects directly to a drillbit
+ private final boolean ownsZkConnection;
+ private final boolean ownsEventLoopGroup;
+ private final boolean ownsExecutor;
+
+ // if the following variables are set during construction, they are not
overridden during or after #connect call
+ // otherwise, they are set to defaults during #connect call
private EventLoopGroup eventLoopGroup;
private ExecutorService executor;
+ private boolean supportComplexTypes;
+
+ // the following variables are set during connection, and must not be
overridden later
+ private UserClient client;
+ private UserProperties props;
+ private volatile ClusterCoordinator clusterCoordinator;
--- End diff --
I'll remove modifier, and document that the class is not thread safe.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56456667
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
--- End diff --
How about calciteOperators?
Also, I meant it for all lists and maps here.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56692111
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java
---
@@ -209,6 +212,10 @@ public static long getInitialPlanningMemorySize() {
return INITIAL_OFF_HEAP_ALLOCATION_IN_BYTES;
}
+ public boolean isTypeInferenceEnabled() {
+return options.getOption(TYPE_INFERENCE.getOptionName()).bool_val;
--- End diff --
```java
return option.getOption(TYPE_INFERENCE);
```
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56692131
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,93 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.OptionManager;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List calciteOperators = Lists.newArrayList();
+ private final List drillOperatorsWithoutInference =
Lists.newArrayList();
+ private final List drillOperatorsWithInference =
Lists.newArrayList();
+ private final Map calciteToWrapper =
Maps.newIdentityHashMap();
+
+ private final ArrayListMultimap
drillOperatorsWithoutInferenceMap = ArrayListMultimap.create();
+ private final ArrayListMultimap
drillOperatorsWithInferenceMap = ArrayListMultimap.create();
- public DrillOperatorTable(FunctionImplementationRegistry registry) {
-operators = Lists.newArrayList();
-operators.addAll(inner.getOperatorList());
+ private final OptionManager systemOptionManager;
+ public DrillOperatorTable(FunctionImplementationRegistry registry,
SystemOptionManager systemOptionManager) {
--- End diff --
`OptionManager systemOptionManager`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426814
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java
---
@@ -100,8 +108,13 @@ public void onMatch(RelOptRuleCall ruleCall) {
*/
private boolean containsAvgStddevVarCall(List
aggCallList) {
for (AggregateCall call : aggCallList) {
- if (call.getAggregation() instanceof SqlAvgAggFunction
- || call.getAggregation() instanceof SqlSumAggFunction) {
+ SqlAggFunction sqlAggFunction = call.getAggregation();
+ if(sqlAggFunction instanceof DrillCalciteSqlWrapper) {
--- End diff --
Would a utility class that does the unwrapping be helpful?
Different util functions unwrap different operators.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426751
--- Diff:
contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java
---
@@ -43,4 +43,32 @@ public void testEncode() throws Exception {
.baselineValues(new Object[] { null })
.go();
}
+
+ @Test
+ public void testReflect() throws Exception {
+final String query = "select reflect('java.lang.Math', 'round', cast(2
as float)) as col \n" +
--- End diff --
How are these queries testing inference?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431301
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
+ private final List operatorsDefault = Lists.newArrayList();
+ private final List operatorsInferernce =
Lists.newArrayList();
+ private final Map calciteToWrapper =
Maps.newIdentityHashMap();
+
+ private final ArrayListMultimap opMapDefault =
ArrayListMultimap.create();
+ private final ArrayListMultimap opMapInferernce =
ArrayListMultimap.create();
+
+ private final SystemOptionManager systemOptionManager;
public DrillOperatorTable(FunctionImplementationRegistry registry) {
-operators = Lists.newArrayList();
-operators.addAll(inner.getOperatorList());
+this(registry, null);
--- End diff --
Is this to get around unit tests? If so, can you please deprecate this
c'tor?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431177
--- Diff:
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/planner/sql/HiveUDFOperator.java
---
@@ -18,28 +18,20 @@
package org.apache.drill.exec.planner.sql;
-import com.fasterxml.jackson.databind.type.TypeFactory;
-import org.apache.calcite.rel.type.RelDataType;
-import org.apache.calcite.rel.type.RelDataTypeFactory;
-import org.apache.calcite.sql.SqlCall;
import org.apache.calcite.sql.SqlCallBinding;
import org.apache.calcite.sql.SqlFunction;
import org.apache.calcite.sql.SqlFunctionCategory;
import org.apache.calcite.sql.SqlIdentifier;
import org.apache.calcite.sql.SqlOperandCountRange;
import org.apache.calcite.sql.SqlOperator;
-import org.apache.calcite.sql.SqlOperatorBinding;
import org.apache.calcite.sql.parser.SqlParserPos;
import org.apache.calcite.sql.type.SqlOperandCountRanges;
import org.apache.calcite.sql.type.SqlOperandTypeChecker;
-import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.sql.validate.SqlValidator;
-import org.apache.calcite.sql.validate.SqlValidatorScope;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
public class HiveUDFOperator extends SqlFunction {
-
- public HiveUDFOperator(String name) {
-super(new SqlIdentifier(name, SqlParserPos.ZERO),
DynamicReturnType.INSTANCE, null, new ArgChecker(), null,
+ public HiveUDFOperator(String name, SqlReturnTypeInference
sqlReturnTypeInference) {
+super(new SqlIdentifier(name, SqlParserPos.ZERO),
sqlReturnTypeInference, null, new ArgChecker(), null,
--- End diff --
Use ArgChecker.INSTANCE (and make that final)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426971
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperatorNotInfer.java
---
@@ -0,0 +1,76 @@
+/**
+ * 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 org.apache.drill.exec.planner.sql;
+
+import com.google.common.base.Preconditions;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlOperatorBinding;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.SqlValidatorScope;
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.exec.expr.fn.DrillFuncHolder;
+
+import java.util.ArrayList;
+
+public class DrillSqlOperatorNotInfer extends DrillSqlOperator {
--- End diff --
Nit: rename
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426767
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -92,23 +118,106 @@ public DrillFunctionRegistry(ScanResult
classpathScan) {
}
public int size(){
-return methods.size();
+return registeredFunctions.size();
}
/** Returns functions with given name. Function name is case
insensitive. */
public List getMethods(String name) {
-return this.methods.get(name.toLowerCase());
+return this.registeredFunctions.get(name.toLowerCase());
}
public void register(DrillOperatorTable operatorTable) {
--- End diff --
Also, is there a way without registering the two variants of the same UDF?
Implementations of PluggableFunctionRegistry need to know call
operatorTable#addInference and operatorTable#addDefault methods.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431148
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlAggOperator.java
---
@@ -17,47 +17,76 @@
*/
package org.apache.drill.exec.planner.sql;
-import java.util.List;
-
-import org.apache.calcite.rel.type.RelDataType;
-import org.apache.calcite.rel.type.RelDataTypeFactory;
+import com.google.common.collect.Lists;
import org.apache.calcite.sql.SqlAggFunction;
-import org.apache.calcite.sql.SqlCall;
import org.apache.calcite.sql.SqlFunctionCategory;
import org.apache.calcite.sql.SqlIdentifier;
import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.parser.SqlParserPos;
-import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.sql.validate.SqlValidator;
-import org.apache.calcite.sql.validate.SqlValidatorScope;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.drill.exec.expr.fn.DrillFuncHolder;
-import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.List;
public class DrillSqlAggOperator extends SqlAggFunction {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillSqlAggOperator.class);
+ // private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillSqlAggOperator.class);
+ private final List functions;
-
- public DrillSqlAggOperator(String name, int argCount) {
-super(name, new SqlIdentifier(name, SqlParserPos.ZERO),
SqlKind.OTHER_FUNCTION, DynamicReturnType.INSTANCE, null, new
Checker(argCount), SqlFunctionCategory.USER_DEFINED_FUNCTION);
+ protected DrillSqlAggOperator(String name, List
functions, int argCountMin, int argCountMax, SqlReturnTypeInference
sqlReturnTypeInference) {
+super(name,
+new SqlIdentifier(name, SqlParserPos.ZERO),
+SqlKind.OTHER_FUNCTION,
+sqlReturnTypeInference,
+null,
+Checker.getChecker(argCountMin, argCountMax),
+SqlFunctionCategory.USER_DEFINED_FUNCTION);
+this.functions = functions;
}
- @Override
- public RelDataType deriveType(SqlValidator validator, SqlValidatorScope
scope, SqlCall call) {
-return getAny(validator.getTypeFactory());
+ private DrillSqlAggOperator(String name, List
functions, int argCountMin, int argCountMax) {
+this(name,
+functions,
+argCountMin,
+argCountMax,
+TypeInferenceUtils.getDrillSqlReturnTypeInference(
+name,
+functions));
}
- private RelDataType getAny(RelDataTypeFactory factory){
-return factory.createSqlType(SqlTypeName.ANY);
-//return new RelDataTypeDrillImpl(new RelDataTypeHolder(), factory);
+ public List getFunctions() {
+return functions;
}
-// @Override
-// public List getParameterTypes(RelDataTypeFactory
typeFactory) {
-//return ImmutableList.of(typeFactory.createSqlType(SqlTypeName.ANY));
-// }
-//
-// @Override
-// public RelDataType getReturnType(RelDataTypeFactory typeFactory) {
-//return getAny(typeFactory);
-// }
+ public static class DrillSqlAggOperatorBuilder {
+private String name;
+private final List functions = Lists.newArrayList();
+private int argCountMin = Integer.MAX_VALUE;
+private int argCountMax = Integer.MIN_VALUE;
+private boolean isDeterministic = true;
--- End diff --
unused?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431228
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerPhase.java ---
@@ -135,6 +135,16 @@ public RuleSet getRules(OptimizerRulesContext context,
Collection
}
},
+ SUM_CONVERSION("Convert SUM to $SUM0") {
--- End diff --
Is this phase enforced?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56427872
--- Diff:
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionRegistry.java
---
@@ -70,7 +85,8 @@ public HiveFunctionRegistry(DrillConfig config) {
@Override
public void register(DrillOperatorTable operatorTable) {
for (String name : Sets.union(methodsGenericUDF.asMap().keySet(),
methodsUDF.asMap().keySet())) {
- operatorTable.add(name, new HiveUDFOperator(name.toUpperCase()));
+ operatorTable.addDefault(name, new
HiveUDFOperatorNotInfer(name.toUpperCase()));
--- End diff --
Please update the docs for PluggableFunctionRegistry accordingly.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426918
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -62,11 +123,70 @@ public void lookupOperatorOverloads(SqlIdentifier
opName, SqlFunctionCategory ca
@Override
public List getOperatorList() {
-return operators;
+final List sqlOperators = Lists.newArrayList();
+sqlOperators.addAll(operatorsCalcite);
+if(isEnableInference()) {
+ sqlOperators.addAll(operatorsInferernce);
+} else {
+ sqlOperators.addAll(operatorsDefault);
+}
+
+return sqlOperators;
}
// Get the list of SqlOperator's with the given name.
public List getSqlOperator(String name) {
-return opMap.get(name.toLowerCase());
+if(isEnableInference()) {
+ return opMapInferernce.get(name.toLowerCase());
+} else {
+ return opMapDefault.get(name.toLowerCase());
+}
+ }
+
+ private void populateWrappedCalciteOperators() {
+for(SqlOperator calciteOperator : inner.getOperatorList()) {
+ final SqlOperator wrapper;
+ if(calciteOperator instanceof SqlAggFunction) {
+wrapper = new DrillCalciteSqlAggFunctionWrapper((SqlAggFunction)
calciteOperator,
+getFunctionListWithInference(calciteOperator.getName()));
+ } else if(calciteOperator instanceof SqlFunction) {
+wrapper = new DrillCalciteSqlFunctionWrapper((SqlFunction)
calciteOperator,
+getFunctionListWithInference(calciteOperator.getName()));
+ } else {
+final String drillOpName =
FunctionCallFactory.replaceOpWithFuncName(calciteOperator.getName());
+final List drillFuncHolders =
getFunctionListWithInference(drillOpName);
+if(drillFuncHolders.isEmpty() || calciteOperator ==
SqlStdOperatorTable.UNARY_MINUS || calciteOperator ==
SqlStdOperatorTable.UNARY_PLUS) {
+ continue;
+}
+
+wrapper = new DrillCalciteSqlOperatorWrapper(calciteOperator,
drillOpName, drillFuncHolders);
+ }
+ calciteToWrapper.put(calciteOperator, wrapper);
+}
+ }
+
+ private List getFunctionListWithInference(String name) {
+final List functions = Lists.newArrayList();
+for(SqlOperator sqlOperator : opMapInferernce.get(name.toLowerCase()))
{
+ if(sqlOperator instanceof DrillSqlOperator) {
+final List list = ((DrillSqlOperator)
sqlOperator).getFunctions();
+if(list != null) {
+ functions.addAll(list);
+}
+ }
+
+ if(sqlOperator instanceof DrillSqlAggOperator) {
+final List list = ((DrillSqlAggOperator)
sqlOperator).getFunctions();
+if(list != null) {
+ functions.addAll(list);
+}
+ }
+}
+return functions;
+ }
+
+ private boolean isEnableInference() {
--- End diff --
Nit: rename to isInferenceEnabled
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426779
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -92,23 +118,106 @@ public DrillFunctionRegistry(ScanResult
classpathScan) {
}
public int size(){
-return methods.size();
+return registeredFunctions.size();
}
/** Returns functions with given name. Function name is case
insensitive. */
public List getMethods(String name) {
-return this.methods.get(name.toLowerCase());
+return this.registeredFunctions.get(name.toLowerCase());
}
public void register(DrillOperatorTable operatorTable) {
+registerForInference(operatorTable);
--- End diff --
Nit: the default is to infer. So these function names are confusing.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431206
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -92,23 +118,106 @@ public DrillFunctionRegistry(ScanResult
classpathScan) {
}
public int size(){
-return methods.size();
+return registeredFunctions.size();
}
/** Returns functions with given name. Function name is case
insensitive. */
public List getMethods(String name) {
-return this.methods.get(name.toLowerCase());
+return this.registeredFunctions.get(name.toLowerCase());
}
public void register(DrillOperatorTable operatorTable) {
+registerForInference(operatorTable);
+registerForDefault(operatorTable);
+ }
+
+ public void registerForInference(DrillOperatorTable operatorTable) {
+final Map map =
Maps.newHashMap();
+final Map
mapAgg = Maps.newHashMap();
+for (Entry> function :
registeredFunctions.asMap().entrySet()) {
+ final ArrayListMultimap, DrillFuncHolder>
functions = ArrayListMultimap.create();
+ final ArrayListMultimap aggregateFunctions
= ArrayListMultimap.create();
+ final String name = function.getKey().toUpperCase();
+ boolean isDeterministic = true;
+ for (DrillFuncHolder func : function.getValue()) {
+final int paramCount = func.getParamCount();
+if(func.isAggregating()) {
+ aggregateFunctions.put(paramCount, func);
+} else {
+ final Pair argNumberRange;
+ if(drillFuncToRange.containsKey(name)) {
+argNumberRange = drillFuncToRange.get(name);
+ } else {
+argNumberRange = Pair.of(func.getParamCount(),
func.getParamCount());
+ }
+ functions.put(argNumberRange, func);
+}
+
+if(!func.isDeterministic()) {
+ isDeterministic = false;
+}
+ }
+ for (Entry, Collection>
entry : functions.asMap().entrySet()) {
+final Pair range = entry.getKey();
+final int max = range.getRight();
+final int min = range.getLeft();
+if(map.containsKey(name)) {
--- End diff --
Nit: simplify if..else
```java
if (!map.containsKey(name) ) {
map.put(name, new
DrillSqlOperator.DrillSqlOperatorBuilder().setName(name));
}
map.get(name)...;
```
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426782
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -92,23 +118,106 @@ public DrillFunctionRegistry(ScanResult
classpathScan) {
}
public int size(){
-return methods.size();
+return registeredFunctions.size();
}
/** Returns functions with given name. Function name is case
insensitive. */
public List getMethods(String name) {
-return this.methods.get(name.toLowerCase());
+return this.registeredFunctions.get(name.toLowerCase());
}
public void register(DrillOperatorTable operatorTable) {
+registerForInference(operatorTable);
+registerForDefault(operatorTable);
+ }
+
+ public void registerForInference(DrillOperatorTable operatorTable) {
--- End diff --
These register methods should be private.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431255
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillAvgVarianceConvertlet.java
---
@@ -40,7 +43,16 @@
public class DrillAvgVarianceConvertlet implements SqlRexConvertlet {
private final SqlAvgAggFunction.Subtype subtype;
- private static final DrillSqlOperator CastHighOp = new
DrillSqlOperator("CastHigh", 1, false);
+ private static final DrillSqlOperator CastHighOp = new
DrillSqlOperator("CastHigh", 1, false,
+ new SqlReturnTypeInference() {
+@Override
+public RelDataType inferReturnType(SqlOperatorBinding opBinding) {
+ return TypeInferenceUtils.createCalciteTypeWithNullability(
+ opBinding.getTypeFactory(),
+ SqlTypeName.ANY,
--- End diff --
(1) Can the return type be inferred based on TypeCastRules?
(2) Are *Convertlet part of validation phase? If not, what guarantees that
return type at validation and at execution are same?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426901
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
+ private final List operatorsDefault = Lists.newArrayList();
+ private final List operatorsInferernce =
Lists.newArrayList();
+ private final Map calciteToWrapper =
Maps.newIdentityHashMap();
+
+ private final ArrayListMultimap opMapDefault =
ArrayListMultimap.create();
+ private final ArrayListMultimap opMapInferernce =
ArrayListMultimap.create();
+
+ private final SystemOptionManager systemOptionManager;
public DrillOperatorTable(FunctionImplementationRegistry registry) {
-operators = Lists.newArrayList();
-operators.addAll(inner.getOperatorList());
+this(registry, null);
+ }
+ public DrillOperatorTable(FunctionImplementationRegistry registry,
SystemOptionManager systemOptionManager) {
registry.register(this);
+operatorsCalcite.addAll(inner.getOperatorList());
+populateWrappedCalciteOperators();
+this.systemOptionManager = systemOptionManager;
+ }
+
+ public void addDefault(String name, SqlOperator op) {
--- End diff --
From names, the functionality of addDefault and addInference is not obvious.
Please add docs to these methods and class, since implementations of
pluggable function registry have to use these methods.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56427110
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/TypeInferenceUtils.java
---
@@ -0,0 +1,740 @@
+/**
+ * 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 org.apache.drill.exec.planner.sql;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+
+import org.apache.calcite.avatica.util.TimeUnit;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlCharStringLiteral;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIntervalQualifier;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlOperatorBinding;
+import org.apache.calcite.sql.SqlRankFunction;
+import org.apache.calcite.sql.fun.SqlAvgAggFunction;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.type.SqlTypeName;
+
+import org.apache.drill.common.expression.ExpressionPosition;
+import org.apache.drill.common.expression.FunctionCall;
+import org.apache.drill.common.expression.FunctionCallFactory;
+import org.apache.drill.common.expression.LogicalExpression;
+import org.apache.drill.common.expression.MajorTypeInLogicalExpression;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.common.types.Types;
+import org.apache.drill.exec.expr.TypeHelper;
+import org.apache.drill.exec.expr.fn.DrillFuncHolder;
+import org.apache.drill.exec.resolver.FunctionResolver;
+import org.apache.drill.exec.resolver.FunctionResolverFactory;
+import org.apache.drill.exec.resolver.TypeCastRules;
+
+import java.util.List;
+
+public class TypeInferenceUtils {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(TypeInferenceUtils.class);
+
+ public static final TypeProtos.MajorType UNKNOWN_TYPE =
TypeProtos.MajorType.getDefaultInstance();
+ private static final ImmutableMap
DRILL_TO_CALCITE_TYPE_MAPPING = ImmutableMap. builder()
--- End diff --
Nit: wrap around to next line (here and below)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431169
--- Diff:
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionRegistry.java
---
@@ -204,4 +220,46 @@ private HiveFuncHolder matchAndCreateUDFHolder(String
udfName,
return null;
}
+ public class HiveSqlReturnTypeInference implements
SqlReturnTypeInference {
+private HiveSqlReturnTypeInference() {
+
+}
+
+@Override
+public RelDataType inferReturnType(SqlOperatorBinding opBinding) {
+ for (RelDataType type : opBinding.collectOperandTypes()) {
+final TypeProtos.MinorType minorType =
TypeInferenceUtils.getDrillTypeFromCalciteType(type);
+if(minorType == TypeProtos.MinorType.LATE) {
+ return opBinding.getTypeFactory()
+ .createTypeWithNullability(
+
opBinding.getTypeFactory().createSqlType(SqlTypeName.ANY),
+ true);
+}
+ }
+
+ final FunctionCall functionCall =
TypeInferenceUtils.convertSqlOperatorBindingToFunctionCall(opBinding);
+ final HiveFuncHolder hiveFuncHolder = getFunction(functionCall);
+ if(hiveFuncHolder == null) {
+String operandTypes = "";
--- End diff --
Nit: use StringBuilder
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56433120
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
+ private final List operatorsDefault = Lists.newArrayList();
--- End diff --
Also, do we need so many structures to track all this?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56692124
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillAvgVarianceConvertlet.java
---
@@ -40,7 +43,16 @@
public class DrillAvgVarianceConvertlet implements SqlRexConvertlet {
private final SqlAvgAggFunction.Subtype subtype;
- private static final DrillSqlOperator CastHighOp = new
DrillSqlOperator("CastHigh", 1, false);
+ private static final DrillSqlOperator CastHighOp = new
DrillSqlOperator("CastHigh", 1, false,
+ new SqlReturnTypeInference() {
+@Override
+public RelDataType inferReturnType(SqlOperatorBinding opBinding) {
+ return TypeInferenceUtils.createCalciteTypeWithNullability(
+ opBinding.getTypeFactory(),
+ SqlTypeName.ANY,
--- End diff --
So regarding 2, what guarantees that return types at validation and at
execution are same, since we add a `CastHigh` after validation?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56692104
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -92,23 +114,100 @@ public DrillFunctionRegistry(ScanResult
classpathScan) {
}
public int size(){
-return methods.size();
+return registeredFunctions.size();
}
/** Returns functions with given name. Function name is case
insensitive. */
public List getMethods(String name) {
-return this.methods.get(name.toLowerCase());
+return this.registeredFunctions.get(name.toLowerCase());
}
public void register(DrillOperatorTable operatorTable) {
+registerOperatorsWithInference(operatorTable);
+registerOperatorsWithoutInference(operatorTable);
+ }
+
+ private void registerOperatorsWithInference(DrillOperatorTable
operatorTable) {
+final Map map =
Maps.newHashMap();
+final Map
mapAgg = Maps.newHashMap();
+for (Entry> function :
registeredFunctions.asMap().entrySet()) {
+ final ArrayListMultimap, DrillFuncHolder>
functions = ArrayListMultimap.create();
+ final ArrayListMultimap aggregateFunctions
= ArrayListMultimap.create();
+ final String name = function.getKey().toUpperCase();
+ boolean isDeterministic = true;
+ for (DrillFuncHolder func : function.getValue()) {
+final int paramCount = func.getParamCount();
+if(func.isAggregating()) {
+ aggregateFunctions.put(paramCount, func);
+} else {
+ final Pair argNumberRange;
+ if(registeredFuncNameToArgRange.containsKey(name)) {
+argNumberRange = registeredFuncNameToArgRange.get(name);
+ } else {
+argNumberRange = Pair.of(func.getParamCount(),
func.getParamCount());
+ }
+ functions.put(argNumberRange, func);
+}
+
+if(!func.isDeterministic()) {
+ isDeterministic = false;
+}
+ }
+ for (Entry, Collection>
entry : functions.asMap().entrySet()) {
+final Pair range = entry.getKey();
+final int max = range.getRight();
+final int min = range.getLeft();
+if(!map.containsKey(name)) {
+ map.put(name, new DrillSqlOperator.DrillSqlOperatorBuilder()
+ .setName(name));
+}
+
+final DrillSqlOperator.DrillSqlOperatorBuilder
drillSqlOperatorBuilder = map.get(name);
+drillSqlOperatorBuilder
+.addFunctions(entry.getValue())
+.setArgumentCount(min, max)
+.setDeterministic(isDeterministic);
+ }
+ for (Entry> entry :
aggregateFunctions.asMap().entrySet()) {
+if(mapAgg.containsKey(name)) {
--- End diff --
simplify this `if..else` as well.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/397#issuecomment-198462911 +1 I have some minor comments. Also, I think a utility class that unwraps *SqlWrapper classes would be helpful. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431197
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
---
@@ -17,40 +17,66 @@
*/
package org.apache.drill.exec.expr.fn;
-import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.List;
+import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.commons.lang3.tuple.Pair;
import
org.apache.drill.common.scanner.persistence.AnnotatedClassDescriptor;
import org.apache.drill.common.scanner.persistence.ScanResult;
-import org.apache.drill.exec.expr.DrillFunc;
+import org.apache.drill.common.types.TypeProtos;
import org.apache.drill.exec.planner.logical.DrillConstExecutor;
import org.apache.drill.exec.planner.sql.DrillOperatorTable;
import org.apache.drill.exec.planner.sql.DrillSqlAggOperator;
+import org.apache.drill.exec.planner.sql.DrillSqlAggOperatorNotInfer;
import org.apache.drill.exec.planner.sql.DrillSqlOperator;
import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.Sets;
+import org.apache.drill.exec.planner.sql.DrillSqlOperatorNotInfer;
+/**
+ * Registry of Drill functions.
+ */
public class DrillFunctionRegistry {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillFunctionRegistry.class);
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillFunctionRegistry.class);
- private ArrayListMultimap methods =
ArrayListMultimap.create();
+ // key: function name (lowercase) value: list of functions with that name
+ private final ArrayListMultimap
registeredFunctions = ArrayListMultimap.create();
- /* Hash map to prevent registering functions with exactly matching
signatures
- * key: Function Name + Input's Major Type
- * Value: Class name where function is implemented
- */
- private HashMap functionSignatureMap = new HashMap<>();
+ private static final ImmutableMap>
drillFuncToRange = ImmutableMap.> builder()
--- End diff --
Nit: rename
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426878
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
--- End diff --
Nits: better names for these variables?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56431299
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
---
@@ -26,34 +33,88 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.drill.exec.planner.physical.PlannerSettings;
+import org.apache.drill.exec.server.options.SystemOptionManager;
import java.util.List;
+import java.util.Map;
+/**
+ * Implementation of {@link SqlOperatorTable} that contains standard
operators and functions provided through
+ * {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
+ */
public class DrillOperatorTable extends SqlStdOperatorTable {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
-
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner =
SqlStdOperatorTable.instance();
- private List operators;
- private ArrayListMultimap opMap =
ArrayListMultimap.create();
+ private final List operatorsCalcite = Lists.newArrayList();
+ private final List operatorsDefault = Lists.newArrayList();
+ private final List operatorsInferernce =
Lists.newArrayList();
+ private final Map calciteToWrapper =
Maps.newIdentityHashMap();
+
+ private final ArrayListMultimap opMapDefault =
ArrayListMultimap.create();
+ private final ArrayListMultimap opMapInferernce =
ArrayListMultimap.create();
+
+ private final SystemOptionManager systemOptionManager;
--- End diff --
Use interface `OptionManager systemOptions;`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426746
--- Diff:
contrib/storage-hive/core/src/main/java/org/apache/drill/exec/planner/sql/HiveUDFOperatorNotInfer.java
---
@@ -0,0 +1,44 @@
+/**
+ * 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 org.apache.drill.exec.planner.sql;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlOperatorBinding;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.SqlValidatorScope;
+
+public class HiveUDFOperatorNotInfer extends HiveUDFOperator {
--- End diff --
Nit: better class name? HiveUDFOperatorWithoutInference?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4372 review
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/397#discussion_r56426944
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlAggOperatorNotInfer.java
---
@@ -0,0 +1,43 @@
+/**
+ * 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 org.apache.drill.exec.planner.sql;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.SqlValidatorScope;
+import org.apache.drill.exec.expr.fn.DrillFuncHolder;
+
+import java.util.ArrayList;
+
+public class DrillSqlAggOperatorNotInfer extends DrillSqlAggOperator {
--- End diff --
Nit: DrillSqlAggOperatorWithoutInference?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3623: For limit 0 queries, use a shorter...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/405#issuecomment-199962117 I have addressed @jinfengni 's and @hsuanyi 's comments [here](https://github.com/sudheeshkatkam/drill/commit/e4cfdfa9b0562d52ac07f6d80860a82fa8baba40) [I force pushed to this branch and somehow their comments are not referenced in this PR any longer.] --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3623: For limit 0 queries, use a shorter...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/405#discussion_r57047786
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestLimit0.java
---
@@ -0,0 +1,677 @@
+/**
+ * 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 org.apache.drill.exec.physical.impl.limit;
+
+import com.google.common.collect.Lists;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.drill.BaseTestQuery;
+import org.apache.drill.PlanTestBase;
+import org.apache.drill.common.expression.SchemaPath;
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.common.types.Types;
+import org.joda.time.DateTime;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.util.List;
+
+public class TestLimit0 extends BaseTestQuery {
+
+ private static final String viewName = "limitZeroEmployeeView";
+
+ private static String wrapLimit0(final String query) {
+return "SELECT * FROM (" + query + ") LZT LIMIT 0";
+ }
+
+ @BeforeClass
+ public static void createView() throws Exception {
+test("USE dfs_test.tmp");
+test(String.format("CREATE OR REPLACE VIEW %s AS SELECT " +
+"CAST(employee_id AS INT) AS employee_id, " +
+"CAST(full_name AS VARCHAR(25)) AS full_name, " +
+"CAST(position_id AS INTEGER) AS position_id, " +
+"CAST(department_id AS BIGINT) AS department_id," +
+"CAST(birth_date AS DATE) AS birth_date, " +
+"CAST(hire_date AS TIMESTAMP) AS hire_date, " +
+"CAST(salary AS DOUBLE) AS salary, " +
+"CAST(salary AS FLOAT) AS fsalary, " +
+"CAST((CASE WHEN marital_status = 'S' THEN true ELSE false END) AS
BOOLEAN) AS single, " +
+"CAST(education_level AS VARCHAR(60)) AS education_level," +
+"CAST(gender AS CHAR) AS gender " +
+"FROM cp.`employee.json` " +
+"ORDER BY employee_id " +
+"LIMIT 1;", viewName));
+// { "employee_id":1,"full_name":"Sheri
Nowmer","first_name":"Sheri","last_name":"Nowmer","position_id":1,
+//
"position_title":"President","store_id":0,"department_id":1,"birth_date":"1961-08-26",
+// "hire_date":"1994-12-01
00:00:00.0","end_date":null,"salary":8.,"supervisor_id":0,
+// "education_level":"Graduate
Degree","marital_status":"S","gender":"F","management_role":"Senior Management"
}
+ }
+
+ @AfterClass
+ public static void tearDownView() throws Exception {
+test("DROP VIEW " + viewName + ";");
+ }
+
+ // SIMPLE QUERIES
+
+ @Test
+ public void infoSchema() throws Exception {
+testBuilder()
+.sqlQuery(String.format("DESCRIBE %s", viewName))
+.unOrdered()
+.baselineColumns("COLUMN_NAME", "DATA_TYPE", "IS_NULLABLE")
+.baselineValues("employee_id", "INTEGER", "YES")
+.baselineValues("full_name", "CHARACTER VARYING", "YES")
+.baselineValues("position_id", "INTEGER", "YES")
+.baselineValues("department_id", "BIGINT", "YES")
+.baselineValues("birth_date", "DATE", "YES")
+
[GitHub] drill pull request: DRILL-3623: For limit 0 queries, use a shorter...
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/405#issuecomment-200026538
Thank you for the reviews.
All regression tests passed; I am running unit tests right now.
Note that, the `planner.enable_limit0_optimization` option is disabled by
default. To summarize (and document) the limitations:
If, during validation, the planner is able to resolve that the types of the
columns (i.e. types are non late binding), the shorter execution path is taken.
Some types are excluded:
+ DECIMAL type is not fully supported in general.
+ VARBINARY is not fully tested.
+ MAP, ARRAY are currently not exposed to the planner.
+ TINYINT, SMALLINT are defined in the Drill type system but have been
turned off for now.
+ SYMBOL, MULTISET, DISTINCT, STRUCTURED, ROW, OTHER, CURSOR, COLUMN_LIST
are Calcite types currently not supported by Drill, nor defined in the Drill
type list.
Three scenarios when the planner can do type resolution during validation:
+ Queries on Hive tables
+ Queries with explicit casts on table columns, example: `SELECT CAST(col1
AS BIGINT), ABS(CAST(col2 AS INTEGER)) FROM table;`
+ Queries on views with casts on table columns
In the latter two cases, the schema of the query with LIMIT 0 clause has
relaxed nullability compared to the query without the LIMIT 0 clause. Example:
Say the schema definition of the Parquet file (`numbers.parquet`) is:
```
message Numbers {
required int col1;
optional int col2;
}
```
Since the view definition does not specify nullability of columns, and
schema of a parquet file is not yet leveraged by Drill's planner:
```
CREATE VIEW dfs.tmp.mynumbers AS SELECT CAST(col1 AS INTEGER) as col1,
CAST(col2 AS INTEGER) AS col2 FROM dfs.tmp.`numbers.parquet`;
```
(1) For query with LIMIT 0 clause, since the file/ metadata is not read,
Drill assumes the nullability of both columns is
[`columnNullable`](https://docs.oracle.com/javase/7/docs/api/java/sql/ResultSetMetaData.html#columnNullable).
```
SELECT col1, col2 FROM dfs.tmp.mynumbers LIMIT 0;
```
(2) For query without LIMIT 0 clause, since the file is read, Drill knows
the nullability of `col1` is
[`columnNoNulls`](https://docs.oracle.com/javase/7/docs/api/java/sql/ResultSetMetaData.html#columnNoNulls),
and `col2` is
[`columnNullable`](https://docs.oracle.com/javase/7/docs/api/java/sql/ResultSetMetaData.html#columnNullable).
```
SELECT col1, col2 FROM dfs.tmp.mynumbers LIMIT 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r57426094
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java
---
@@ -50,8 +52,12 @@
public BootStrapContext(DrillConfig config, ScanResult classpathScan) {
this.config = config;
this.classpathScan = classpathScan;
-this.loop =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
"BitServer-");
-this.loop2 =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
"BitClient-");
+this.controlLoopGroup =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
--- End diff --
Done. I am hesitant to lower the control loop size until all blocking calls
in handling requests are removed (e.g. cancelling an executor while fragment is
starting up).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r57426103
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -74,73 +74,148 @@
/**
* Thin wrapper around a UserClient that handles connect/close and
transforms
* String into ByteBuf.
+ *
+ * Use the builder class ({@link DrillClient.Builder}) to build objects of
this class.
+ * E.g.
+ *
+ * DrillClient client = DrillClient.newBuilder()
+ * .setConfig(...)
+ * .setIsDirectConnection(true)
+ * .build();
+ *
*/
public class DrillClient implements Closeable, ConnectionThrottle {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillClient.class);
private final DrillConfig config;
- private UserClient client;
- private UserProperties props = null;
- private volatile ClusterCoordinator clusterCoordinator;
- private volatile boolean connected = false;
private final BufferAllocator allocator;
- private int reconnectTimes;
- private int reconnectDelay;
- private boolean supportComplexTypes;
- private final boolean ownsZkConnection;
+ private final boolean isDirectConnection;
+ private final int reconnectTimes;
+ private final int reconnectDelay;
+
+ // checks if this client owns these resources (used when closing)
private final boolean ownsAllocator;
- private final boolean isDirectConnection; // true if the connection
bypasses zookeeper and connects directly to a drillbit
+ private final boolean ownsZkConnection;
+ private final boolean ownsEventLoopGroup;
+ private final boolean ownsExecutor;
+
+ // if the following variables are set during construction, they are not
overridden during or after #connect call
+ // otherwise, they are set to defaults during #connect call
private EventLoopGroup eventLoopGroup;
private ExecutorService executor;
+ private boolean supportComplexTypes;
+
+ // the following variables are set during connection, and must not be
overridden later
+ private UserClient client;
+ private UserProperties props;
+ private volatile ClusterCoordinator clusterCoordinator;
--- End diff --
Updated the doc.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r57426098
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/client/DrillClientSystemTest.java
---
@@ -51,7 +51,7 @@ public void tearDownTest() {
@Test
public void testSubmitPlanSingleNode() throws Exception {
startCluster(1);
-DrillClient client = new DrillClient();
+DrillClient client = DrillClient.newBuilder().build();
--- End diff --
Done.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r57426117
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -74,73 +74,148 @@
/**
* Thin wrapper around a UserClient that handles connect/close and
transforms
* String into ByteBuf.
+ *
+ * Use the builder class ({@link DrillClient.Builder}) to build objects of
this class.
+ * E.g.
+ *
+ * DrillClient client = DrillClient.newBuilder()
+ * .setConfig(...)
+ * .setIsDirectConnection(true)
+ * .build();
+ *
*/
public class DrillClient implements Closeable, ConnectionThrottle {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillClient.class);
private final DrillConfig config;
- private UserClient client;
- private UserProperties props = null;
- private volatile ClusterCoordinator clusterCoordinator;
- private volatile boolean connected = false;
private final BufferAllocator allocator;
- private int reconnectTimes;
- private int reconnectDelay;
- private boolean supportComplexTypes;
- private final boolean ownsZkConnection;
+ private final boolean isDirectConnection;
+ private final int reconnectTimes;
+ private final int reconnectDelay;
+
+ // checks if this client owns these resources (used when closing)
private final boolean ownsAllocator;
- private final boolean isDirectConnection; // true if the connection
bypasses zookeeper and connects directly to a drillbit
+ private final boolean ownsZkConnection;
+ private final boolean ownsEventLoopGroup;
+ private final boolean ownsExecutor;
+
+ // if the following variables are set during construction, they are not
overridden during or after #connect call
+ // otherwise, they are set to defaults during #connect call
private EventLoopGroup eventLoopGroup;
private ExecutorService executor;
+ private boolean supportComplexTypes;
+
+ // the following variables are set during connection, and must not be
overridden later
+ private UserClient client;
+ private UserProperties props;
+ private volatile ClusterCoordinator clusterCoordinator;
+ private volatile boolean connected; // = false
- public DrillClient() throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient() {
this(DrillConfig.create(), false);
}
- public DrillClient(boolean isDirect) throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(boolean isDirect) {
this(DrillConfig.create(), isDirect);
}
- public DrillClient(String fileName) throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(String fileName) {
this(DrillConfig.create(fileName), false);
}
- public DrillClient(DrillConfig config) throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config) {
this(config, null, false);
}
- public DrillClient(DrillConfig config, boolean isDirect)
- throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, boolean isDirect) {
this(config, null, isDirect);
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator)
-throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator) {
this(config, coordinator, null, false);
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator,
boolean isDirect)
-throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator,
boolean isDirect) {
this(config, coordinator, null, isDirect);
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator,
BufferAllocator allocator)
- throws OutOfMemoryException {
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/429#issuecomment-201191189 Please re-review as I have more changes; I hope splitting up the changes into 4 commits helps. @jacques-n can you also review rpc.control package info? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/429#discussion_r57470730
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java
---
@@ -50,8 +52,12 @@
public BootStrapContext(DrillConfig config, ScanResult classpathScan) {
this.config = config;
this.classpathScan = classpathScan;
-this.loop =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
"BitServer-");
-this.loop2 =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
"BitClient-");
+this.controlLoopGroup =
TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS),
--- End diff --
Yes, I missed the obvious :) I'll change them to 5 each.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: First pass at test re-factoring
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/135#issuecomment-203057745 @aleph-zero I see the last update was a few months back. I know @jaltekruse mentioned about this in the hangout a while back. What is the latest on this (I forget)? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4523: Disallow using loopback address in...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/445#discussion_r58234800
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java
---
@@ -177,6 +182,18 @@ public void run() {
});
}
+ /** If we are in distributed mode, disallow using loopback address to
register new drillbit
+ * unless zookeeper is on the same node with drillbit. */
+ private void checkLoopbackAddress(String address) throws
DrillbitStartupException, UnknownHostException {
+if (isDistributedMode) {
+ String coordAddress =
StringUtils.substringBefore(config.getString(ExecConstants.ZK_CONNECTION), ":");
+ if (!InetAddress.getByName(coordAddress).isLoopbackAddress()
--- End diff --
In distributed mode, no bit should be allowed to register with a loopback
address.
If bit A is allowed to register with a loopback address and bit B tries to
communicate with bit A, bit B would be sending messages to itself.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/442#discussion_r58285085
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection,
Channel channel) {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
String msg;
- if(local!=null) {
+ if(local != null) {
msg = String.format("Channel closed %s <--> %s.", local, remote);
}else{
msg = String.format("Channel closed %s <--> %s.",
future.channel().localAddress(), future.channel().remoteAddress());
}
- if (RpcBus.this.isClient()) {
-if(local != null) {
- logger.info(String.format(msg));
-}
- } else {
-queue.channelClosed(new ChannelClosedException(msg));
- }
+ logger.info(msg); // should we leave this at info level ?
+
+ queue.channelClosed(new ChannelClosedException(msg));
--- End diff --
@adeneche @jacques-n correct me if I am wrong.
Per my understanding, this logic is incomplete, with or without this
change. Let's looks at bit-to-bit comm.
There is one CoordinationQueue **for each instance** of RpcBus (\*Server,
\*Client classes inherit from RpcBus). Also, this queue is used by requestors
to listen to outcomes of requests.
1. DataClient <--> DataServer. DataClient is always the requestor, and
there can be at most two data connections between two bits (A --> B and B -->
A).
a. Since a DataClient is created per connection, the client's queue
contains outcomes of requests to one DataServer. When this connection closes,
failing all RPC outcomes in the queue makes sense.
b. There is only one instance of DataServer per Drillbit, and so **one
server queue**. Since DataServer never makes requests, this queue should be
empty. `queue.channelClosed(...)` should be a noop.
2. ControlClient <--> ControlServer. This communication is peer-to-peer
i.e. ControlServer and ControlClient can make requests and handle requests. The
bit initiating a connection is the ControlClient, and its peer is the
ControlServer for lifetime of this connection. There can be at most one
connection between two bits (A <--> B), and messages are sent both ways. Now,
assume a connection is made.
a. On the ControlClient side, the queue contains outcomes of requests to
one ControlServer. When this connection closes, failing all RPC outcomes in the
queue makes sense.
b. There is only one instance of ControlServer per Drillbit, and so **one
server queue**. However, ControlServer can make requests to other bits, and to
multiple clients! So this queue can contain outcomes from multiple connections
and `queue.channelClosed(...)` fails outcomes of requests from **all**
connections??
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/442#discussion_r58296230
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection,
Channel channel) {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
String msg;
- if(local!=null) {
+ if(local != null) {
msg = String.format("Channel closed %s <--> %s.", local, remote);
}else{
msg = String.format("Channel closed %s <--> %s.",
future.channel().localAddress(), future.channel().remoteAddress());
}
- if (RpcBus.this.isClient()) {
-if(local != null) {
- logger.info(String.format(msg));
-}
- } else {
-queue.channelClosed(new ChannelClosedException(msg));
- }
+ logger.info(msg); // should we leave this at info level ?
+
+ queue.channelClosed(new ChannelClosedException(msg));
--- End diff --
Makes sense that this problem shows up in UserServer <--> UserClient comm.;
not just [DRILL-3763](https://issues.apache.org/jira/browse/DRILL-3763), I
think [DRILL-3823](https://issues.apache.org/jira/browse/DRILL-3823),
[DRILL-3833](https://issues.apache.org/jira/browse/DRILL-3833),
[DRILL-4170](https://issues.apache.org/jira/browse/DRILL-4170) and
[DRILL-4298](https://issues.apache.org/jira/browse/DRILL-4298) will also be
resolved :)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/442#discussion_r58316779
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection,
Channel channel) {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
String msg;
- if(local!=null) {
+ if(local != null) {
msg = String.format("Channel closed %s <--> %s.", local, remote);
}else{
msg = String.format("Channel closed %s <--> %s.",
future.channel().localAddress(), future.channel().remoteAddress());
}
- if (RpcBus.this.isClient()) {
-if(local != null) {
- logger.info(String.format(msg));
-}
- } else {
-queue.channelClosed(new ChannelClosedException(msg));
- }
+ logger.info(msg); // should we leave this at info level ?
+
+ queue.channelClosed(new ChannelClosedException(msg));
--- End diff --
I meant [DRILL-3826](https://issues.apache.org/jira/browse/DRILL-3826).
Looks like you noted that
[DRILL-3241](https://issues.apache.org/jira/browse/DRILL-3241) is also related.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/442#issuecomment-205412305 +0 Why do we need a work around? I think Jacques is suggesting to move the logic to RemoteConnection, which is much cleaner. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/442#discussion_r58482936
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection,
Channel channel) {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
String msg;
- if(local!=null) {
+ if(local != null) {
msg = String.format("Channel closed %s <--> %s.", local, remote);
}else{
msg = String.format("Channel closed %s <--> %s.",
future.channel().localAddress(), future.channel().remoteAddress());
}
- if (RpcBus.this.isClient()) {
-if(local != null) {
- logger.info(String.format(msg));
-}
- } else {
-queue.channelClosed(new ChannelClosedException(msg));
- }
+ logger.info(msg); // should we leave this at info level ?
+
+ queue.channelClosed(new ChannelClosedException(msg),
future.channel());
clientConnection.close();
--- End diff --
Slightly unrelated: is this `clientConnection.close();` call unnecessary,
since this listener is meant to handle the underlying channel closure?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4523: Disallow using loopback address in...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/445#discussion_r58483415
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java
---
@@ -177,6 +182,13 @@ public void run() {
});
}
+ /** If in distributed mode, disallow using loopback address to register
new drillbit. */
+ private void checkLoopbackAddress(String address) throws
DrillbitStartupException, UnknownHostException {
+if (isDistributedMode &&
InetAddress.getByName(address).isLoopbackAddress()) {
+ throw new DrillbitStartupException("Loopback address is not allowed
to be registered in distributed mode");
--- End diff --
How about "Drillbit is disallowed to bind to loopback address in
distributed mode."?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active queries if server conn...
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/460 DRILL-3743: Fail active queries if server connection is closed + Remove dead code + Improve error and logging messages @jacques-n @adeneche please review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-3743 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/460.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 #460 commit 2c6d156d7e9285abb915941942f2c6e9642b4511 Author: Sudheesh Katkam Date: 2016-04-05T04:13:10Z DRILL-3743: Fail active queries if server connection is closed + Remove dead code + Improve error and logging messages --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58587275
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
---
@@ -278,49 +297,31 @@ public void queryIdArrived(QueryId queryId) {
private class SubmissionListener extends BaseRpcOutcomeListener
{
private final UserResultsListener resultsListener;
-private final RemoteConnection connection;
-private final ChannelFuture closeFuture;
-private final ChannelClosedListener closeListener;
private final AtomicBoolean isTerminal = new AtomicBoolean(false);
-public SubmissionListener(RemoteConnection connection,
UserResultsListener resultsListener) {
- super();
+public SubmissionListener(UserResultsListener resultsListener) {
this.resultsListener = resultsListener;
- this.connection = connection;
- this.closeFuture = connection.getChannel().closeFuture();
- this.closeListener = new ChannelClosedListener();
- closeFuture.addListener(closeListener);
-}
-
-private class ChannelClosedListener implements
GenericFutureListener> {
-
- @Override
- public void operationComplete(Future future) throws Exception {
-resultsListener.submissionFailed(UserException.connectionError()
-.message("Connection %s closed unexpectedly.",
connection.getName())
-.build(logger));
- }
-
}
@Override
public void failed(RpcException ex) {
if (!isTerminal.compareAndSet(false, true)) {
+logger.warn("Received multiple outcomes (success/failure) while
submitting query.");
return;
}
- closeFuture.removeListener(closeListener);
-
resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
-
+ resultsListener.submissionFailed(UserException.systemError(ex)
+ .message("Unexpected failure while submitting query to a
Drillbit.")
--- End diff --
will update.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58587852
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
-@Override
-public void interrupted(final InterruptedException ex) {
--- End diff --
Submissions are asynchronous; no thread is waiting.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58589199
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
---
@@ -189,20 +218,10 @@ private UserResultsListener
newUserResultsListener(QueryId queryId) {
if ( null == resultsListener ) {
resultsListener = bl;
}
- // TODO: Is there a more direct way to detect a Query ID in
whatever state this string comparison detects?
- if ( queryId.toString().isEmpty() ) {
--- End diff --
Yes; if query id string was empty, there would be failures on the server
side (like exchanges).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4523: Disallow using loopback address in...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/445#issuecomment-205945078 +1 How did you test this? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58595373
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
---
@@ -351,20 +352,5 @@ public void success(QueryId queryId, ByteBuf buf) {
}
}
}
-
-@Override
-public void interrupted(final InterruptedException ex) {
--- End diff --
My bad, I did not consider back pressure. The warning message is
misleading. I'll update the warning and user exception message.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/460#discussion_r58613516
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
---
@@ -278,49 +297,31 @@ public void queryIdArrived(QueryId queryId) {
private class SubmissionListener extends BaseRpcOutcomeListener
{
private final UserResultsListener resultsListener;
-private final RemoteConnection connection;
-private final ChannelFuture closeFuture;
-private final ChannelClosedListener closeListener;
private final AtomicBoolean isTerminal = new AtomicBoolean(false);
-public SubmissionListener(RemoteConnection connection,
UserResultsListener resultsListener) {
- super();
+public SubmissionListener(UserResultsListener resultsListener) {
this.resultsListener = resultsListener;
- this.connection = connection;
- this.closeFuture = connection.getChannel().closeFuture();
- this.closeListener = new ChannelClosedListener();
- closeFuture.addListener(closeListener);
-}
-
-private class ChannelClosedListener implements
GenericFutureListener> {
-
- @Override
- public void operationComplete(Future future) throws Exception {
-resultsListener.submissionFailed(UserException.connectionError()
-.message("Connection %s closed unexpectedly.",
connection.getName())
-.build(logger));
- }
-
}
@Override
public void failed(RpcException ex) {
if (!isTerminal.compareAndSet(false, true)) {
+logger.warn("Received multiple outcomes (success/failure) while
submitting query.");
return;
}
- closeFuture.removeListener(closeListener);
-
resultsListener.submissionFailed(UserException.systemError(ex).build(logger));
-
+ resultsListener.submissionFailed(UserException.systemError(ex)
+ .message("Unexpected failure while submitting query to a
Drillbit.")
--- End diff --
Looks like system errors preserve the original message. So this message was
being ignored. I'll add the message to context.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3743: Fail active listeners if server co...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/460#issuecomment-206027992 Addressed review comments. + Move new changes + Improve error and logging messages --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4588: Enable JMX reporting
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/469 DRILL-4588: Enable JMX reporting @parthchandra please review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4588 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/469.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 #469 commit 4500cc9075c72622972e81939551ada2dfdca0a5 Author: Sudheesh Katkam Date: 2016-04-06T23:41:52Z DRILL-4588: Enable JMX reporting --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806614
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
+ private final AtomicBoolean acceptMessage = new AtomicBoolean(true);
- private final PositiveAtomicInteger circularInt = new
PositiveAtomicInteger();
--- End diff --
Remove PositiveAtomicIneteger class.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806644
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
+ private final AtomicBoolean acceptMessage = new AtomicBoolean(true);
- private final PositiveAtomicInteger circularInt = new
PositiveAtomicInteger();
- private final Map> map;
+ /** Access to map must be protected. **/
+ private final IntObjectHashMap> map;
- public CoordinationQueue(int segmentSize, int segmentCount) {
-map = new ConcurrentHashMap>(segmentSize,
0.75f, segmentCount);
+ public RequestIdMap() {
+map = new IntObjectHashMap>();
}
void channelClosed(Throwable ex) {
+acceptMessage.set(false);
if (ex != null) {
- RpcException e;
- if (ex instanceof RpcException) {
-e = (RpcException) ex;
- } else {
-e = new RpcException(ex);
+ final RpcException e = RpcException.mapException(ex);
+ synchronized (map) {
+map.forEach(new Closer(e));
+map.clear();
}
- for (RpcOutcome f : map.values()) {
-f.setException(e);
+}
+ }
+
+ private class Closer implements IntObjectProcedure> {
+final RpcException exception;
+
+public Closer(RpcException exception) {
+ this.exception = exception;
+}
+
+@Override
+public void apply(int key, RpcOutcome value) {
+ try{
+value.setException(exception);
+ }catch(Exception e){
+logger.warn("Failure while attempting to fail rpc response.", e);
}
}
+
}
- public ChannelListenerWithCoordinationId get(RpcOutcomeListener
handler, Class clazz, RemoteConnection connection) {
-int i = circularInt.getNext();
+ public ChannelListenerWithCoordinationId
createNewRpcListener(RpcOutcomeListener handler, Class clazz,
+ RemoteConnection connection) {
+int i = value.incrementAndGet();
RpcListener future = new RpcListener(handler, clazz, i,
connection);
-Object old = map.put(i, future);
-if (old != null) {
- throw new IllegalStateException(
- "You attempted to reuse a coordination id when the previous
coordination id has not been removed. This is likely rpc future callback
memory leak.");
+final Object old;
+synchronized (map) {
+ Preconditions.checkArgument(acceptMessage.get(),
+ "Attempted to send a message when connection is no longer
valid.");
+ old = map.put(i, future);
}
+Preconditions.checkArgument(old == null,
--- End diff --
Not required, since numbers are no longer reused?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806604
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java
---
@@ -282,15 +283,19 @@ public void interrupted(final InterruptedException
ex) {
private class ClientHandshakeHandler extends
AbstractHandshakeHandler {
-public ClientHandshakeHandler() {
+private final R connection;
+
+public ClientHandshakeHandler(R connection) {
super(BasicClient.this.handshakeType,
BasicClient.this.handshakeParser);
+ Preconditions.checkNotNull(connection);
--- End diff --
`this.connection = Preconditions.checkNotNull(connection);`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806623
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
+ private final AtomicBoolean acceptMessage = new AtomicBoolean(true);
- private final PositiveAtomicInteger circularInt = new
PositiveAtomicInteger();
- private final Map> map;
+ /** Access to map must be protected. **/
+ private final IntObjectHashMap> map;
- public CoordinationQueue(int segmentSize, int segmentCount) {
-map = new ConcurrentHashMap>(segmentSize,
0.75f, segmentCount);
+ public RequestIdMap() {
+map = new IntObjectHashMap>();
}
void channelClosed(Throwable ex) {
+acceptMessage.set(false);
if (ex != null) {
- RpcException e;
- if (ex instanceof RpcException) {
-e = (RpcException) ex;
- } else {
-e = new RpcException(ex);
+ final RpcException e = RpcException.mapException(ex);
+ synchronized (map) {
+map.forEach(new Closer(e));
+map.clear();
}
- for (RpcOutcome f : map.values()) {
-f.setException(e);
+}
+ }
+
+ private class Closer implements IntObjectProcedure> {
+final RpcException exception;
+
+public Closer(RpcException exception) {
+ this.exception = exception;
+}
+
+@Override
+public void apply(int key, RpcOutcome value) {
+ try{
--- End diff --
Inconsistent spacing here and below.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806611
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
--- End diff --
private
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806647
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -158,22 +157,16 @@ public ChannelClosedHandler(C clientConnection,
Channel channel) {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
- String msg;
+ final String msg;
+
if(local!=null) {
msg = String.format("Channel closed %s <--> %s.", local, remote);
}else{
msg = String.format("Channel closed %s <--> %s.",
future.channel().localAddress(), future.channel().remoteAddress());
}
- if (RpcBus.this.isClient()) {
--- End diff --
`isClient` method is no longer used. Remove the method.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58806651
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java ---
@@ -261,6 +251,7 @@ public void execute(Runnable command) {
public InboundHandler(C connection) {
super();
+ Preconditions.checkNotNull(connection);
--- End diff --
`this.connection = Preconditions.checkNotNull(connection);`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58821752
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
--- End diff --
How about `coordinationIdCounter` and `isOpen`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58821789
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
+ private final AtomicBoolean acceptMessage = new AtomicBoolean(true);
- private final PositiveAtomicInteger circularInt = new
PositiveAtomicInteger();
- private final Map> map;
+ /** Access to map must be protected. **/
+ private final IntObjectHashMap> map;
- public CoordinationQueue(int segmentSize, int segmentCount) {
-map = new ConcurrentHashMap>(segmentSize,
0.75f, segmentCount);
+ public RequestIdMap() {
+map = new IntObjectHashMap>();
}
void channelClosed(Throwable ex) {
+acceptMessage.set(false);
if (ex != null) {
- RpcException e;
- if (ex instanceof RpcException) {
-e = (RpcException) ex;
- } else {
-e = new RpcException(ex);
+ final RpcException e = RpcException.mapException(ex);
+ synchronized (map) {
+map.forEach(new Closer(e));
+map.clear();
}
- for (RpcOutcome f : map.values()) {
-f.setException(e);
+}
+ }
+
+ private class Closer implements IntObjectProcedure> {
--- End diff --
Better class name? `SetExceptionProcedure`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58821796
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -20,51 +20,82 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelFuture;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.drill.common.exceptions.UserRemoteException;
import org.apache.drill.exec.proto.UserBitShared.DrillPBError;
+import com.carrotsearch.hppc.IntObjectHashMap;
+import com.carrotsearch.hppc.procedures.IntObjectProcedure;
+import com.google.common.base.Preconditions;
+
/**
- * Manages the creation of rpc futures for a particular socket.
+ * Manages the creation of rpc futures for a particular socket <--> socket
+ * connection. Generally speaking, there will be two threads working with
this
+ * class (the socket thread and the Request generating thread).
Synchronization
+ * is simple with the map being the only thing that is protected.
Everything
+ * else works via Atomic variables.
*/
-public class CoordinationQueue {
- static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(CoordinationQueue.class);
+class RequestIdMap {
+ static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(RequestIdMap.class);
+
+ private final AtomicInteger value = new AtomicInteger();
+ private final AtomicBoolean acceptMessage = new AtomicBoolean(true);
- private final PositiveAtomicInteger circularInt = new
PositiveAtomicInteger();
- private final Map> map;
+ /** Access to map must be protected. **/
+ private final IntObjectHashMap> map;
- public CoordinationQueue(int segmentSize, int segmentCount) {
-map = new ConcurrentHashMap>(segmentSize,
0.75f, segmentCount);
+ public RequestIdMap() {
+map = new IntObjectHashMap>();
}
void channelClosed(Throwable ex) {
+acceptMessage.set(false);
if (ex != null) {
- RpcException e;
- if (ex instanceof RpcException) {
-e = (RpcException) ex;
- } else {
-e = new RpcException(ex);
+ final RpcException e = RpcException.mapException(ex);
+ synchronized (map) {
+map.forEach(new Closer(e));
+map.clear();
}
- for (RpcOutcome f : map.values()) {
-f.setException(e);
+}
+ }
+
+ private class Closer implements IntObjectProcedure> {
+final RpcException exception;
+
+public Closer(RpcException exception) {
+ this.exception = exception;
+}
+
+@Override
+public void apply(int key, RpcOutcome value) {
+ try{
+value.setException(exception);
+ }catch(Exception e){
+logger.warn("Failure while attempting to fail rpc response.", e);
}
}
+
}
- public ChannelListenerWithCoordinationId get(RpcOutcomeListener
handler, Class clazz, RemoteConnection connection) {
-int i = circularInt.getNext();
+ public ChannelListenerWithCoordinationId
createNewRpcListener(RpcOutcomeListener handler, Class clazz,
+ RemoteConnection connection) {
+int i = value.incrementAndGet();
RpcListener future = new RpcListener(handler, clazz, i,
connection);
-Object old = map.put(i, future);
-if (old != null) {
- throw new IllegalStateException(
- "You attempted to reuse a coordination id when the previous
coordination id has not been removed. This is likely rpc future callback
memory leak.");
+final Object old;
+synchronized (map) {
+ Preconditions.checkArgument(acceptMessage.get(),
--- End diff --
Make this check first statement in the method?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/463#discussion_r58821834
--- Diff:
exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java ---
@@ -84,7 +115,7 @@ public void operationComplete(ChannelFuture future)
throws Exception {
if (!future.isSuccess()) {
removeFromMap(coordinationId);
if (future.channel().isActive()) {
- throw new RpcException("Future failed") ;
+ throw new RpcException("Future failed");
--- End diff --
Since the future did not succeed, should this
`setException(future.cause())`? There would be no outcome for the `handler`
otherwise, right?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/463#issuecomment-207025128 Overall change looks good; I have some minor comments. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58956992
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/FindLimit0Visitor.java
---
@@ -116,4 +131,33 @@ public RelNode visit(LogicalMinus minus) {
public RelNode visit(LogicalUnion union) {
return union;
}
+
+ /**
+ * Visitor to scan the RelNode tree and find if it contains any Scans
that require hard distribution requirements.
+ */
+ public static class FindHardDistributionScans extends RelShuttleImpl {
--- End diff --
private
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58956976
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/ParallelizationParameters.java
---
@@ -0,0 +1,32 @@
+/**
+ * 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 org.apache.drill.exec.planner.fragment;
+
+/**
+ * Interface to implement for passing parameters to {@link
FragmentParallelizer}
+ */
+public interface ParallelizationParameters {
+
+ long getSliceTarget();
--- End diff --
Doc?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58956963
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/HardAffinityFragmentParallelizer.java
---
@@ -0,0 +1,134 @@
+/**
+ * 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 org.apache.drill.exec.planner.fragment;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.physical.EndpointAffinity;
+import org.apache.drill.exec.physical.PhysicalOperatorSetupException;
+import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
+import org.slf4j.Logger;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+/**
+ * Implementation of {@link FragmentParallelizer} where fragment requires
running on a given set of endpoints. Width
+ * per node is depended on the affinity to the endpoint and total width
(calculated using costs)
+ */
+public class HardAffinityFragmentParallelizer implements
FragmentParallelizer {
+ private static final Logger logger =
org.slf4j.LoggerFactory.getLogger(HardAffinityFragmentParallelizer.class);
+
+ public static final HardAffinityFragmentParallelizer INSTANCE = new
HardAffinityFragmentParallelizer();
+
+ private static String EOL = System.getProperty("line.separator");
--- End diff --
What's special about this class?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58957028
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/FindLimit0Visitor.java
---
@@ -43,7 +50,15 @@
public static boolean containsLimit0(RelNode rel) {
FindLimit0Visitor visitor = new FindLimit0Visitor();
rel.accept(visitor);
-return visitor.isContains();
+
+if (!visitor.isContains()) {
+ return false;
+}
+
+final FindHardDistributionScans hdVisitor = new
FindHardDistributionScans();
+rel.accept(hdVisitor);
+// Can't optimize limit 0 if the query contains a table which has hard
distribution requirement.
+return !hdVisitor.contains();
--- End diff --
LGTM
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58956969
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/HardAffinityFragmentParallelizer.java
---
@@ -0,0 +1,134 @@
+/**
+ * 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 org.apache.drill.exec.planner.fragment;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.physical.EndpointAffinity;
+import org.apache.drill.exec.physical.PhysicalOperatorSetupException;
+import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
+import org.slf4j.Logger;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+/**
+ * Implementation of {@link FragmentParallelizer} where fragment requires
running on a given set of endpoints. Width
+ * per node is depended on the affinity to the endpoint and total width
(calculated using costs)
+ */
+public class HardAffinityFragmentParallelizer implements
FragmentParallelizer {
+ private static final Logger logger =
org.slf4j.LoggerFactory.getLogger(HardAffinityFragmentParallelizer.class);
+
+ public static final HardAffinityFragmentParallelizer INSTANCE = new
HardAffinityFragmentParallelizer();
+
+ private static String EOL = System.getProperty("line.separator");
+
+ private HardAffinityFragmentParallelizer() { /* singleton */}
+
+ @Override
+ public void parallelizeFragment(final Wrapper fragmentWrapper, final
ParallelizationParameters parameters,
+ final Collection activeEndpoints) throws
PhysicalOperatorSetupException {
+
+final Stats stats = fragmentWrapper.getStats();
+final ParallelizationInfo pInfo = stats.getParallelizationInfo();
+
+// Go through the affinity map and extract the endpoints that have
mandatory assignment requirement
+final Map endpointPool =
Maps.newHashMap();
+for(Entry entry :
pInfo.getEndpointAffinityMap().entrySet()) {
+ if (entry.getValue().isAssignmentRequired()) {
+endpointPool.put(entry.getKey(), entry.getValue());
+ }
+}
+
+// Step 1: Find the width taking into various parameters
+// 1.1. Find the parallelization based on cost. Use max cost of all
operators in this fragment; this is consistent
+// with the calculation that ExcessiveExchangeRemover uses.
+int width = (int) Math.ceil(stats.getMaxCost() /
parameters.getSliceTarget());
+
+// 1.2. Make sure the width is at least the number of endpoints that
require an assignment
+width = Math.max(endpointPool.size(), width);
+
+// 1.3. Cap the parallelization width by fragment level width limit
and system level per query width limit
+width = Math.max(1, Math.min(width, pInfo.getMaxWidth()));
+checkAndThrow(endpointPool.size() <= width, logger,
+"Number of mandatory endpoints ({}) that require an assignment is
more than the allowed fragment max " +
+"width ({}).", endpointPool.size(), pInfo.getMaxWidth());
+
+// 1.4 Cap the parallelization width by global max query width
+width = Math.max(1, Math.min(width, parameters.getMaxGlobalWidth()));
+checkAndThrow(endpointPool.size() <= width, logger,
+"Number of mandatory endpoints ({}) that require an assignment is
more than the allowed global query " +
+"width ({}).", endpointPool.size(),
parameters.getMaxGlobalWidth());
+
+// 1.5 Cap the parallelization width by max allowed parallelization
per node
+width = Math.max(1, Math.min(width,
endpointPool.size()*parameters.getMaxWidthPerNode()));
+
+// Step 2: Select the endpoints
+final Map endpoints = Maps.newHashMap();
+
+// 2.1 First add each endpoint from the pool o
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58956953
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/DistributionAffinity.java
---
@@ -0,0 +1,63 @@
+/**
+ * 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 org.apache.drill.exec.planner.fragment;
+
+/**
+ * Describes an operator's endpoint assignment requirements. Ordering is
from no assignment requirement to mandatory
+ * assignment requirements. Changes/new addition should keep the order of
increasing restrictive assignment requirement.
+ */
+public enum DistributionAffinity {
+ /**
+ * No affinity to any endpoints. Operator can run on any endpoint.
+ */
+ NONE(SoftAffinityFragmentParallelizer.INSTANCE),
+
+ /**
+ * Operator has soft distribution affinity to one or more endpoints.
Operator performs better when fragments are
+ * assigned to the endpoints with affinity, but not a mandatory
requirement.
+ */
+ SOFT(SoftAffinityFragmentParallelizer.INSTANCE),
+
+ /**
+ * Hard distribution affinity to one or more endpoints. Fragments having
the operator must be scheduled on the nodes
+ * with affinity.
+ */
+ HARD(HardAffinityFragmentParallelizer.INSTANCE);
+
+ private final FragmentParallelizer fragmentParallelizer;
+
+ DistributionAffinity(final FragmentParallelizer fragmentParallelizer) {
+this.fragmentParallelizer = fragmentParallelizer;
+ }
+
+ /**
+ * @return {@link FragmentParallelizer} implementation.
+ */
+ public FragmentParallelizer getFragmentParallelizer() {
+return fragmentParallelizer;
+ }
+
+ /**
+ * Is the current DistributionAffinity less restrictive than the given
DistributionAffinity?
+ * @param distributionAffinity
+ * @return
+ */
+ public boolean isLessRestrictive(final DistributionAffinity
distributionAffinity) {
--- End diff --
`isLessRestrictiveThan`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/403#discussion_r58957037
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/planner/fragment/TestHardAffinityFragmentParallelizer.java
---
@@ -0,0 +1,192 @@
+/**
+ * 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 org.apache.drill.exec.planner.fragment;
+
+import com.google.common.collect.HashMultiset;
+import com.google.common.collect.ImmutableList;
+import mockit.Mocked;
+import mockit.NonStrictExpectations;
+import org.apache.drill.exec.physical.EndpointAffinity;
+import org.apache.drill.exec.physical.base.PhysicalOperator;
+import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+import static org.apache.drill.exec.ExecConstants.SLICE_TARGET_DEFAULT;
+import static
org.apache.drill.exec.planner.fragment.HardAffinityFragmentParallelizer.INSTANCE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestHardAffinityFragmentParallelizer {
--- End diff --
negative tests?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/474#discussion_r59065316
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java ---
@@ -207,6 +209,28 @@ private void javaPropertiesToSystemOptions() {
}
/**
+ * Disallow registering drillbit when:
+ * 1. version is unknown;
+ * 2. drillbit with different version has been already registered.
+ */
+ private void checkVersion(DrillbitEndpoint endpoint) throws
DrillbitStartupException {
+String currentVersion = endpoint.getVersion();
+if (DrillVersionInfo.UNKNOWN_VERSION.equals(currentVersion)) {
+ throw new DrillbitStartupException("Drillbit version is unknown.");
+}
+
+for (DrillbitEndpoint registeredEnpoint :
coord.getAvailableEndpoints()) {
+ if (!currentVersion.equals(registeredEnpoint.getVersion())) {
--- End diff --
How does this check work with regards to [protobuf backwards
compatibility](https://developers.google.com/protocol-buffers/docs/javatutorial#extending-a-protocol-buffer)?
Endpoints currently do not have a version. So if two bits (A and B), one with
and one without this change, are started, the two start order cases (A after B,
and B after A) should pass.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4258: Add threads, fragments, and querie...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/479#discussion_r59787703
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/ThreadStatCollector.java
---
@@ -0,0 +1,124 @@
+/**
+ * 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 org.apache.drill.exec.ops;
+
+import com.carrotsearch.hppc.LongObjectHashMap;
+import com.carrotsearch.hppc.procedures.LongObjectProcedure;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.Deque;
+import java.util.Iterator;
+import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentLinkedDeque;
+
+public class ThreadStatCollector implements Runnable {
--- End diff --
There is a thread (`WorkManager.StatusThread`) that does periodic tasks
(currently updates fragment statuses), how about we add this as another task?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4606: Add DrillClient.Builder class
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/480 DRILL-4606: Add DrillClient.Builder class You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4606 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/480.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 #480 commit ed4d81202071bf698ef7791ec571988dd8d32c6c Author: Sudheesh Katkam Date: 2016-03-09T18:34:26Z DRILL-4606: HYGIENE + Merge DrillAutoCloseables and AuthCloseables + Remove unused imports commit 560820c6646e944a441b2b3d3dfac0e5b2b513c7 Author: Sudheesh Katkam Date: 2016-04-15T05:25:02Z DRILL-4606: CORE + Add DrillClient.Builder helper class to create DrillClient objects + Deprecate 8 constructors and DrillClientFactory + Reorganization and documentation in DrillClient + Reuse user loop group in REST clients --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/429#issuecomment-210564638 There are two independent changes in this patch. So I moved the DrillClient changes to #480 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4606: Add DrillClient.Builder class
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/480#issuecomment-210564893 @hnfgns can you please review? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4584: JDBC/ODBC Client IP in Drill audit...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/475#issuecomment-212979375 +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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4623: Disable epoll transport by default
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/486 DRILL-4623: Disable epoll transport by default @jacques-n can you please review? Note: although this patch disables epoll as default transport, deployments that use another env file (e.g. `/etc/drill/conf/drill-env.sh`) would use epoll. Or disabling could be hard coded? You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4623 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/486.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 #486 commit 537b1decd7227ded9a971d2a9d4d62369255fc92 Author: Sudheesh Katkam Date: 2016-04-21T17:28:29Z DRILL-4623: Disable epoll transport by default --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61117837
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
--- End diff --
Why use an option (mutable), and not `DrillVersionInfo.getVersion()`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61121843
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
--- End diff --
Hmm there's a problem then.. Default values
(`DrillVersionInfo.getVersion()`) are not put in the persistent store, meaning
each Drillbit would start with its own version, and the intended warning will
not be generated.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61121899
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
+
+Map props = Maps.newLinkedHashMap();
+props.put("Cluster Version", version);
+props.put("Number of Drill Bits", work.getContext().getBits().size());
+CoordinationProtos.DrillbitEndpoint currentEndpoint =
work.getContext().getEndpoint();
+String address = currentEndpoint.getAddress();
+props.put("Data Port Address", address + ":" +
currentEndpoint.getDataPort());
+props.put("User Port Address", address + ":" +
currentEndpoint.getUserPort());
+props.put("Control Port Address", address + ":" +
currentEndpoint.getControlPort());
+props.put("Maximum Direct Memory", DrillConfig.getMaxDirectMemory());
+
+return new Stats(props, collectBits(version));
+ }
+
+ private Collection collectBits(String version) {
+Set bits = Sets.newTreeSet();
+for (CoordinationProtos.DrillbitEndpoint endpoint :
work.getContext().getBits()) {
+ boolean versionMatch = version.equals(endpoint.getVersion());
+ Bit bit = new Bit(endpoint.getAddress(), endpoint.isInitialized(),
endpoint.getVersion(), versionMatch);
+ bits.add(bit);
}
-stats.add(new Stat("Data Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getDataPort()));
-stats.add(new Stat("User Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getUserPort()));
-stats.add(new Stat("Control Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getControlPort()));
-stats.add(new Stat("Maximum Direct Memory",
DrillConfig.getMaxDirectMemory()));
-
-return stats;
+return bits;
}
@XmlRootElement
- public class Stat {
-private String name;
-private Object value;
+ public class Stats {
+private Map props;
+private Collection bits;
@JsonCreator
-public Stat(String name, Object value) {
- this.name = name;
- this.value = value;
+public Stats(Map props, Collection bits) {
+ this.props = props;
+ this.bits = bits;
+}
+
+public Map getProps() {
+ return props;
}
-public String getName() {
- return name;
+public Collection getBits() {
+ return bits;
}
+ }
+
+ public class Bit implements Comparable {
--- End diff --
+ rename to DrillbitInfo
+ static class
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61121890
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
+
+Map props = Maps.newLinkedHashMap();
--- End diff --
Nit: use `final` generously, here and below (including classes).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61122239
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -17,17 +17,56 @@
back
-
-
-
-<#list model as stat>
-
-${stat.getName()}
-${stat.getValue()}
-
-
-
-
+
+ <#list model.getBits() as bit>
+<#if !bit.isVersionMatch()>
+
+×
+Drill bits version mismatch is detected.
--- End diff --
How about `Drillbits in the cluster have different versions.`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61122829
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -17,17 +17,56 @@
back
-
-
-
-<#list model as stat>
-
-${stat.getName()}
-${stat.getValue()}
-
-
-
-
+
+ <#list model.getBits() as bit>
+<#if !bit.isVersionMatch()>
+
+×
+Drill bits version mismatch is detected.
+
+ <#break>
+
+
+
+
+
+ General Info
+
+
+
+<#assign props = model.getProps()>
+<#list props?keys as key>
+
+${key}
+${props[key]}
+
+
+
+
+
+
+
+List of Drill Bits
--- End diff --
Drillbits
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61122919
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -17,17 +17,56 @@
back
-
-
-
-<#list model as stat>
-
-${stat.getName()}
-${stat.getValue()}
-
-
-
-
+
+ <#list model.getBits() as bit>
+<#if !bit.isVersionMatch()>
+
+×
+Drill bits version mismatch is detected.
+
+ <#break>
+
+
+
+
+
+ General Info
+
+
+
+<#assign props = model.getProps()>
+<#list props?keys as key>
+
+${key}
+${props[key]}
+
+
+
+
+
+
+
+List of Drill Bits
+
+
+
+ <#assign i = 1>
+ <#list model.getBits() as bit>
+
+ Bit # ${i}
--- End diff --
Drillbit (my bad for the original Bit)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61121895
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
+
+Map props = Maps.newLinkedHashMap();
+props.put("Cluster Version", version);
+props.put("Number of Drill Bits", work.getContext().getBits().size());
+CoordinationProtos.DrillbitEndpoint currentEndpoint =
work.getContext().getEndpoint();
+String address = currentEndpoint.getAddress();
+props.put("Data Port Address", address + ":" +
currentEndpoint.getDataPort());
+props.put("User Port Address", address + ":" +
currentEndpoint.getUserPort());
+props.put("Control Port Address", address + ":" +
currentEndpoint.getControlPort());
+props.put("Maximum Direct Memory", DrillConfig.getMaxDirectMemory());
+
+return new Stats(props, collectBits(version));
+ }
+
+ private Collection collectBits(String version) {
+Set bits = Sets.newTreeSet();
+for (CoordinationProtos.DrillbitEndpoint endpoint :
work.getContext().getBits()) {
+ boolean versionMatch = version.equals(endpoint.getVersion());
+ Bit bit = new Bit(endpoint.getAddress(), endpoint.isInitialized(),
endpoint.getVersion(), versionMatch);
+ bits.add(bit);
}
-stats.add(new Stat("Data Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getDataPort()));
-stats.add(new Stat("User Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getUserPort()));
-stats.add(new Stat("Control Port Address",
work.getContext().getEndpoint().getAddress() +
- ":" + work.getContext().getEndpoint().getControlPort()));
-stats.add(new Stat("Maximum Direct Memory",
DrillConfig.getMaxDirectMemory()));
-
-return stats;
+return bits;
}
@XmlRootElement
- public class Stat {
-private String name;
-private Object value;
+ public class Stats {
--- End diff --
static class
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61123535
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
--- End diff --
Sadly, that does not happen. Take a look at SystemOptionManager
(specifically put and setOption methods).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4604: Generate warning on Web UI if dril...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/482#discussion_r61127476
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
---
@@ -55,44 +59,89 @@ public Viewable getStats() {
@GET
@Path("/stats.json")
@Produces(MediaType.APPLICATION_JSON)
- public List getStatsJSON() {
-List stats = Lists.newLinkedList();
-stats.add(new Stat("Number of Drill Bits",
work.getContext().getBits().size()));
-int number = 0;
-for (CoordinationProtos.DrillbitEndpoint bit :
work.getContext().getBits()) {
- String initialized = bit.isInitialized() ? " initialized" : " not
initialized";
- stats.add(new Stat("Bit #" + number, bit.getAddress() +
initialized));
- ++number;
+ public Stats getStatsJSON() {
+String version =
work.getContext().getOptionManager().getOption(ExecConstants.CLUSTER_VERSION).string_val;
--- End diff --
The assumption for system options is that default values are same across
drillbits, and so they need not be put in the persistent store.
In this case, the default values across drillbits might not be the same.
This option has a default value of what is returned by
`DrillVersionInfo.getVersion()`, and default values are **not put** in
persistent store. But if an admin modifies the option, the value is put in the
store.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/495 DRILL-4654: Add new metrics to the MetricRegistry + New metrics: running queries, pending queries, completed queries, used memory (root allocator) + Borrow SystemPropertyUtil class from Netty + Configure DrillMetrics params through system properties + Deprecate getMetrics method in contextual objects + Rename "current" to "used" for RPC allocator current memory usage to follow convention You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-4654 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/495.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 #495 commit 1fc205bebaf3c1f20df85996ba49cf98b60e4c9e Author: Sudheesh Katkam Date: 2016-05-04T06:10:47Z DRILL-4654: Add new metrics to the MetricRegistry + New metrics: running queries, pending queries, completed queries, used memory (root allocator) + Borrow SystemPropertyUtil class from Netty + Configure DrillMetrics params through system properties + Deprecate getMetrics method in contextual objects + Rename "current" to "used" for RPC allocator current memory usage to follow convention --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/495#issuecomment-217221522 Good points. I added a comment in code, but it does not completely address your concerns. Unfortunately, not all class (e.g. RootAllocator) can be instrumented for metrics if the single instance is part of a contextual object. Static instance is fine according to [library doc](https://dropwizard.github.io/metrics/3.1.0/getting-started/#the-registry). This means maintaining state in a static object. Is there a workaround? AFAIK we do not provide a way to run multiple drillbits in the same JVM in production use cases. You are right, metrics will conflict for unit tests (as is, right 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4654: Add new metrics to the MetricRegis...
Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/495#issuecomment-219192360 @jaltekruse I updated the PR (reverted deprecation); please review? The commit message has details. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4676: Foreman.moveToState can block fore...
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/503#discussion_r63442039
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -296,7 +295,7 @@ public void run() {
* would wait on the cancelling thread to signal a resume and the
cancelling thread would wait on the Foreman
* to accept events.
*/
- acceptExternalEvents.countDown();
+ stateSwitch.start();
--- End diff --
This defers all state transitions until the thread is done. This changes
the behavior when queuing is enabled, the user will not know if the query has
moved from ENQUEUED to STARTING.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
