You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/03/01 15:08:47 UTC
[GitHub] [ignite] tledkov-gridgain opened a new pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
tledkov-gridgain opened a new pull request #8842:
URL: https://github.com/apache/ignite/pull/8842
…alization not standard SQL functions
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r586245551
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java
##########
@@ -842,15 +843,31 @@ private boolean single(SqlNode sqlNode) {
private void onMessage(UUID nodeId, QueryStartRequest msg) {
assert nodeId != null && msg != null;
- PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
+ try {
+ PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
+
+ List<QueryPlan> qryPlans = queryPlanCache().queryPlan(
+ pctx,
+ new CacheKey(pctx.schemaName(), pctx.query()),
+ this::prepareFragment
+ );
- List<QueryPlan> qryPlans = queryPlanCache().queryPlan(pctx, new CacheKey(pctx.schemaName(), pctx.query()), this::prepareFragment);
+ assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
- assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
+ FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
- FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
+ executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ }
+ catch (Throwable ex) {
+ try {
+ exchangeSvc.sendError(nodeId, msg.queryId(), msg.fragmentId(), ex);
+ }
+ catch (IgniteCheckedException e) {
+ U.error(log, "Error occurred during send error message: " + X.getFullStackTrace(e));
+ }
- executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ throw ex;
Review comment:
```suggestion
/*
* No exceptions are rethrown here to preserve the current thread from being destroyed,
* because other queries may be pinned to the current thread id.
* However, unrecoverable errors must be processed by FailureHandler.
*/
uncaughtException(Thread.currentThread(), e);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r585538358
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java
##########
@@ -842,15 +843,29 @@ private boolean single(SqlNode sqlNode) {
private void onMessage(UUID nodeId, QueryStartRequest msg) {
assert nodeId != null && msg != null;
- PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
+ try {
+ PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
- List<QueryPlan> qryPlans = queryPlanCache().queryPlan(pctx, new CacheKey(pctx.schemaName(), pctx.query()), this::prepareFragment);
+ List<QueryPlan> qryPlans = queryPlanCache().queryPlan(
+ pctx,
+ new CacheKey(pctx.schemaName(), pctx.query()),
+ this::prepareFragment
+ );
- assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
+ assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
- FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
+ FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
- executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ }
+ catch (Throwable ex) {
+ try {
+ exchangeSvc.sendError(nodeId, msg.queryId(), msg.fragmentId(), ex);
Review comment:
It would be ok to skip sending an answer or log an error if ExecService is about to stop.
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r586244941
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/QueryTaskExecutorImpl.java
##########
@@ -74,7 +75,20 @@ public void failureProcessor(FailureProcessor failureProcessor) {
/** {@inheritDoc} */
@Override public void execute(UUID qryId, long fragmentId, Runnable qryTask) {
- stripedThreadPoolExecutor.execute(qryTask, hash(qryId, fragmentId));
+ stripedThreadPoolExecutor.execute(
+ () -> {
+ try {
+ qryTask.run();
+ }
+ catch (Throwable e) {
+ if (X.cause(e, InterruptedException.class) != null)
+ Thread.currentThread().interrupt();
+
Review comment:
```suggestion
/*
* No exceptions are rethrown here to preserve the current thread from being destroyed,
* because other queries may be pinned to the current thread id.
* However, unrecoverable errors must be processed by FailureHandler.
*/
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] tledkov-gridgain closed pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
tledkov-gridgain closed pull request #8842:
URL: https://github.com/apache/ignite/pull/8842
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r585539690
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java
##########
@@ -842,15 +843,29 @@ private boolean single(SqlNode sqlNode) {
private void onMessage(UUID nodeId, QueryStartRequest msg) {
assert nodeId != null && msg != null;
- PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
+ try {
+ PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
- List<QueryPlan> qryPlans = queryPlanCache().queryPlan(pctx, new CacheKey(pctx.schemaName(), pctx.query()), this::prepareFragment);
+ List<QueryPlan> qryPlans = queryPlanCache().queryPlan(
+ pctx,
+ new CacheKey(pctx.schemaName(), pctx.query()),
+ this::prepareFragment
+ );
- assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
+ assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
- FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
+ FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
- executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ }
+ catch (Throwable ex) {
+ try {
+ exchangeSvc.sendError(nodeId, msg.queryId(), msg.fragmentId(), ex);
Review comment:
Error (e.g. OOME) should be rethrown to allow FH to kill current node.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r586775248
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExchangeServiceImpl.java
##########
@@ -197,8 +198,10 @@ protected void onMessage(UUID nodeId, QueryBatchAcknowledgeMessage msg) {
try {
outbox.onAcknowledge(nodeId, msg.batchId());
}
- catch (Throwable t) {
- outbox.onError(t);
+ catch (Throwable e) {
+ outbox.onError(e);
+
+ throw new IgniteException("Unexpected exception", e);
}
}
else if (log.isDebugEnabled()) {
Review comment:
seems like you've missed one more place. PLease see `protected void onMessage(UUID nodeId, QueryBatchMessage msg)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a change in pull request #8842: IGNITE-14229 Calcite bug. Exceptions on QueryStartRequest aren't handled
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #8842:
URL: https://github.com/apache/ignite/pull/8842#discussion_r585617292
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java
##########
@@ -842,15 +843,29 @@ private boolean single(SqlNode sqlNode) {
private void onMessage(UUID nodeId, QueryStartRequest msg) {
assert nodeId != null && msg != null;
- PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
+ try {
+ PlanningContext pctx = createContext(Contexts.empty(), msg.topologyVersion(), nodeId, msg.schema(), msg.root(), msg.parameters());
- List<QueryPlan> qryPlans = queryPlanCache().queryPlan(pctx, new CacheKey(pctx.schemaName(), pctx.query()), this::prepareFragment);
+ List<QueryPlan> qryPlans = queryPlanCache().queryPlan(
+ pctx,
+ new CacheKey(pctx.schemaName(), pctx.query()),
+ this::prepareFragment
+ );
- assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
+ assert qryPlans.size() == 1 && qryPlans.get(0).type() == QueryPlan.Type.FRAGMENT;
- FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
+ FragmentPlan plan = (FragmentPlan)qryPlans.get(0);
- executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ executeFragment(msg.queryId(), plan, pctx, msg.fragmentDescription());
+ }
+ catch (Throwable ex) {
+ try {
+ exchangeSvc.sendError(nodeId, msg.queryId(), msg.fragmentId(), ex);
Review comment:
> It would be ok to skip sending an answer or log an error if ExecService is about to stop.
WDYT?
yes, it absolutely ok. All awaiting queries should be cancelled by originator node when NODE_LEFT event will arise. I'd rather log it with debug or trace level
> Error (e.g. OOME) should be rethrown to allow FH to kill current node.
sounds legit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org