You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by abhipol <gi...@git.apache.org> on 2015/11/18 01:43:27 UTC
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
GitHub user abhipol opened a pull request:
https://github.com/apache/drill/pull/263
DRILL-4015: Update DrillClient and JDBC driver to expose warnings provided via RPC layer
surface our query warnings from operators to JDBC client as a part of query result as well as out of band warnings. Details: https://docs.google.com/document/d/1HwpD3gRbNohpse9zbm3cmLZixGn6EgapulEGrRtKlkA
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/abhipol/drill issues/DRILL-4015
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/263.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 #263
----
commit 137059cd44ec28e8ba3bf2aa73d2c1cbcd55d604
Author: Abhi P <ab...@dremio.com>
Date: 2015-11-17T22:54:56Z
Support for drill warnings in JDBC
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233316
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java ---
@@ -76,6 +77,24 @@
/**
* Maps internal low-level API protocol to {@link UserResultsListener}-level API protocol.
+ * handles query warning messages
+ */
+ public void warningArrived( ByteBuf pBody ) throws RpcException {
+ //parse query warning out of RPCBus
+ final QueryWarning queryWarning = RpcBus.get( pBody, QueryWarning.PARSER );
+ final QueryId queryId = queryWarning.getQueryId();
+ final UserResultsListener resultsListener = newUserResultsListener(queryId);
+
+ try {
+ logger.trace("QRH Warning Arrived: {}", queryWarning.toString());
+ resultsListener.warningsArrived(queryWarning);
+ } catch ( Exception e ) {
--- End diff --
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233734
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java ---
@@ -172,6 +174,14 @@ public FragmentContext(final DrillbitContext dbContext, final PlanFragment fragm
stats = new FragmentStats(allocator, dbContext.getMetrics(), fragment.getAssignment());
bufferManager = new BufferManagerImpl(this.allocator);
+ warnings = new DrillWarningHelper (getFragIdString());
--- End diff --
QueryIdHelper.getFragmentId(getHandle());
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233282
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningHelper.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.exception;
+
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.proto.UserBitShared.WarningMsg;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+
+/**
+ * Helper class to manage warnings for FragmentContext and Foreman
+ */
+public class DrillWarningHelper {
+
+ private List<IdentityHashMap<String, WarningMsg>> warnings;
+ private String fragmentId;
+ int numWarnings = 0;
+
+ public DrillWarningHelper (String fragmentId) {
--- End diff --
Please remove spaces between function names and parentheses.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45385946
--- Diff: protocol/src/main/protobuf/User.proto ---
@@ -27,6 +27,7 @@ enum RpcType {
RESP_FUNCTION_LIST = 9;
QUERY_RESULT = 10; // drillbit is reporting a query status change, most likely a terminal message, to the user
+ QUERY_WARNING = 12; // drillbit is sending a query warning to the user
--- End diff --
In general, I like the idea of the server maintaining backward compatibility for the RPC protocol especially across minor releases. Any changes to the protocol that cannot be made backward compatible should have to wait for a major release.
The C++ client does check for RPC version match but at the moment, the server itself breaks the connection if the RPC version does not match and so we never even get to that point. It is possible for the server to manage this entirely and return the RPC version that the client is asking for, iff the server can maintain backward compatibility.
In this case, the C++client also fails the query if it gets an RPC message it does not recognize. Once again, as you suggest, the server can maintain the client's RPC version in the query context and not send back the warning message if the client is at an older version.
Actually, I'm not sure I've ever seen a need for early termination of queries based on warnings, but it could be useful. Not sure how well any of the JDBC/ODBC tools handle this. But I can see some usefulness there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45386115
--- Diff: protocol/src/main/protobuf/User.proto ---
@@ -27,6 +27,7 @@ enum RpcType {
RESP_FUNCTION_LIST = 9;
QUERY_RESULT = 10; // drillbit is reporting a query status change, most likely a terminal message, to the user
+ QUERY_WARNING = 12; // drillbit is sending a query warning to the user
--- End diff --
If we go with the approach of the server maintaining backward compatibility, then I'd like to test and make sure with the C++ client before we merge 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233366
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -178,6 +180,16 @@ public boolean next() throws SQLException {
@Override
public void close() {
// Note: No already-closed exception for close().
+ try {
+ int cnt = 1;
+ SQLWarning warn = getWarnings();
+ while (warn != null) {
+ System.out.println("WARNING-" + cnt++ + ": " + warn.getMessage());
--- End diff --
Is this sysout going to be removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233287
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningHelper.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.exception;
+
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.proto.UserBitShared.WarningMsg;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+
+/**
+ * Helper class to manage warnings for FragmentContext and Foreman
+ */
+public class DrillWarningHelper {
+
+ private List<IdentityHashMap<String, WarningMsg>> warnings;
+ private String fragmentId;
+ int numWarnings = 0;
+
+ public DrillWarningHelper (String fragmentId) {
+ this.warnings = Lists.newArrayList();
--- End diff --
unnecessary 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233408
--- Diff: protocol/src/main/protobuf/User.proto ---
@@ -27,6 +27,7 @@ enum RpcType {
RESP_FUNCTION_LIST = 9;
QUERY_RESULT = 10; // drillbit is reporting a query status change, most likely a terminal message, to the user
+ QUERY_WARNING = 12; // drillbit is sending a query warning to the user
--- End diff --
Warnings are part of the QueryResult object (QUERY_RESULT RpcType). So why is this type required?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233275
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningHelper.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.exception;
+
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.proto.UserBitShared.WarningMsg;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+
+/**
+ * Helper class to manage warnings for FragmentContext and Foreman
+ */
+public class DrillWarningHelper {
+
+ private List<IdentityHashMap<String, WarningMsg>> warnings;
--- End diff --
finals!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233303
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningTypes.java ---
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.exception;
+
+public interface DrillWarningTypes {
--- End diff --
I am assuming this is for testing purposes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45369955
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -161,6 +162,11 @@ public void sendResult(RpcOutcomeListener<Ack> listener, QueryResult result, boo
send(listener, this, RpcType.QUERY_RESULT, result, Ack.class, allowInEventThread);
}
+ public void sendWarning(RpcOutcomeListener<Ack> listener, QueryWarning warning, boolean allowInEventThread){
--- End diff --
allowInEventThread shouldn't be exposed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233331
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -135,6 +138,7 @@
private final ForemanResult foremanResult = new ForemanResult();
private final ConnectionClosedListener closeListener = new ConnectionClosedListener();
private final ChannelFuture closeFuture;
+ private QueryWarning queryWarning;
--- End diff --
Please move this, and the following methods to the QueryManager where other state is maintained.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45239402
--- Diff: protocol/src/main/protobuf/User.proto ---
@@ -27,6 +27,7 @@ enum RpcType {
RESP_FUNCTION_LIST = 9;
QUERY_RESULT = 10; // drillbit is reporting a query status change, most likely a terminal message, to the user
+ QUERY_WARNING = 12; // drillbit is sending a query warning to the user
--- End diff --
If we include this, we will break the ODBC client backward compatibility. It would be best to leave this out and include a warning object optionally in the QueryResult. Older clients will be able to handle this, they just won't return a warning to the application. Newer clients can be updated to return a warning to the application.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45370022
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -192,6 +196,32 @@ public QueryManager getQueryManager() {
}
/**
+ * Collect warnings messages from all fragments and store them in QueryWarning
+ * @param warnings warning message from a fragment
+ */
+ public void addQueryWarnings (List<WarningMsg> warnings) {
--- End diff --
Need to propose an approach around thread safety guarantees 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233292
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningHelper.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.exception;
+
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.proto.UserBitShared.WarningMsg;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+
+/**
+ * Helper class to manage warnings for FragmentContext and Foreman
+ */
+public class DrillWarningHelper {
+
+ private List<IdentityHashMap<String, WarningMsg>> warnings;
+ private String fragmentId;
+ int numWarnings = 0;
+
+ public DrillWarningHelper (String fragmentId) {
+ this.warnings = Lists.newArrayList();
+ this.fragmentId = fragmentId;
+ }
+
+ /**
+ * Interface for operators to add warnings
+ * @param operatorId operator warning is coming from
+ * @param type pre-defined warning type
+ * @param msg custom warning message
+ */
+ public void addWarning (int operatorId, String type, String msg) {
+ assert (operatorId >= 0 && type != null);
+
+ IdentityHashMap<String, WarningMsg> warningMap = null;
--- End diff --
unnecessary null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45234894
--- Diff: protocol/src/main/protobuf/BitControl.proto ---
@@ -46,6 +46,11 @@ message BitStatus {
message FragmentStatus {
optional exec.shared.MinorFragmentProfile profile = 1;
optional FragmentHandle handle = 2;
+ repeated exec.shared.WarningMsg summary_warnings = 3; //vehicle for summary warnings
+}
+
+message DrillWarning{
--- End diff --
I am assuming this is for testing purposes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233299
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/DrillWarningHelper.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.exception;
+
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.proto.UserBitShared.WarningMsg;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+
+/**
+ * Helper class to manage warnings for FragmentContext and Foreman
+ */
+public class DrillWarningHelper {
+
+ private List<IdentityHashMap<String, WarningMsg>> warnings;
+ private String fragmentId;
+ int numWarnings = 0;
+
+ public DrillWarningHelper (String fragmentId) {
+ this.warnings = Lists.newArrayList();
+ this.fragmentId = fragmentId;
+ }
+
+ /**
+ * Interface for operators to add warnings
+ * @param operatorId operator warning is coming from
+ * @param type pre-defined warning type
+ * @param msg custom warning message
+ */
+ public void addWarning (int operatorId, String type, String msg) {
+ assert (operatorId >= 0 && type != null);
+
+ IdentityHashMap<String, WarningMsg> warningMap = null;
+ if (operatorId < warnings.size()) {
+ // we have received warnings from this operator before, go fetch it
+ warningMap = warnings.get(operatorId);
+ }
+
+ if (warningMap == null) {
+ // first warning for this operator
+ warningMap = new IdentityHashMap<>();
+ }
+
+ WarningMsg wm = warningMap.get(type);
+ if (wm == null) {
+ // first warning of this type from the operator
+ wm = WarningMsg.newBuilder()
+ .setMessage(msg)
+ .setCount(1)
+ .build();
+ numWarnings++;
+ } else {
+ // we have seen this type before from the operator, update it
+ wm = wm.toBuilder()
+ .setMessage(msg)
+ .setCount(wm.getCount() + 1)
+ .build();
+ }
+ warningMap.put(type, wm);
+ warnings.add(operatorId, warningMap);
+ }
+
+ /**
+ * Get Unique warning code
+ * @param operatorId
+ * @param type
+ * @return
+ */
+ public String getWarningCode (int operatorId, String type) {
+ return operatorId + "-" + this.fragmentId + "-" + type;
+ }
+
+ /**
+ * Get flattened list of WarningMsg
+ * @return list of WarningMsg
+ */
+ public List<WarningMsg> getWarnings () {
+ List<WarningMsg> retWarnings = Lists.newArrayList();
+ for (int opId = 0; opId < warnings.size(); opId++) {
+ IdentityHashMap<String, WarningMsg> warning = warnings.get(opId);
+ if (warning == null) {
+ continue;
+ }
+ for (String type : warning.keySet()) {
+ WarningMsg wm = warning.get(type);
+ wm.toBuilder()
+ .setWarningCode(getWarningCode(opId, type))
+ .build();
+ retWarnings.add(wm);
+ }
+ }
+ return retWarnings;
+ }
+
+ public void clearWarnings () {
+ for (IdentityHashMap<String, WarningMsg> warning : warnings) {
+ warning.clear();
+ }
+ warnings.clear();
--- End diff --
Why isn't this clear() sufficient?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45370924
--- Diff: protocol/src/main/protobuf/User.proto ---
@@ -27,6 +27,7 @@ enum RpcType {
RESP_FUNCTION_LIST = 9;
QUERY_RESULT = 10; // drillbit is reporting a query status change, most likely a terminal message, to the user
+ QUERY_WARNING = 12; // drillbit is sending a query warning to the user
--- End diff --
We actually started with only a compound result/warning (and that is also included in this patch). One of the things we want to give is the user should have an ability to prematurely terminate a query if they don't like the warning that we are returning. (For example: we had to create a null field since it didn't exist.) To do that, we may have an early warning that we want to return out of band. For backwards compatibility, we can quell sending these back to the user if they are on an older rpc version.
One question here: we should make the server decide whether the rpc version number is okay (if we don't already). That way we can upgrade server and manage backwards compatibility. If the client is currently checking the RPC version and also terminating if it doesn't like it, we're screwed for backwards compatbility. (My thought is that we should update version number whenever the protocol changes but we need to relax the compatibility check. We can simply decide what is cross-compatible in the code layer. (e.g. v3 works with v2 client but don't send them warnings.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233348
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentStatusReporter.java ---
@@ -98,7 +98,17 @@ void stateChanged(final FragmentState newState) {
}
private void sendStatus(final FragmentStatus status) {
- tunnel.sendFragmentStatus(status);
+ FragmentStatus updatedStatus = status;
+ // grab warnings from fragment context and pass them with fragment status
+ if (context.getWarningHelper().hasWarnings()) {
--- End diff --
Can this be moved to the getStatus 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4015: Update DrillClient and JDBC driver...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/263#discussion_r45233412
--- Diff: protocol/src/main/protobuf/UserBitShared.proto ---
@@ -91,6 +91,18 @@ message DrillPBError{
repeated ParsingError parsing_error = 6; //optional, used when providing location of error within a piece of text.
}
+message WarningMsg{
--- End diff --
WarningMessage?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---