You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by lavjain <gi...@git.apache.org> on 2018/01/30 22:18:31 UTC
[GitHub] incubator-hawq pull request #1334: Don't ignore exceptions during bridge.end...
GitHub user lavjain opened a pull request:
https://github.com/apache/incubator-hawq/pull/1334
Don't ignore exceptions during bridge.endIteration when writing to HDFS
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lavjain/incubator-hawq fix_writable
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/1334.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 #1334
----
commit 36eaff0ed2aea5428aca86331fa515824e4fc328
Author: lavjain <lj...@...>
Date: 2018-01-30T07:16:14Z
Don't ignore exceptions during bridge.endIteration when writing to HDFS
----
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by lavjain <gi...@git.apache.org>.
Github user lavjain closed the pull request at:
https://github.com/apache/incubator-hawq/pull/1334
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by lavjain <gi...@git.apache.org>.
Github user lavjain closed the pull request at:
https://github.com/apache/incubator-hawq/pull/1334
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r166099466
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -143,36 +142,38 @@ private static synchronized Response synchronizedWriteResponse(Bridge bridge,
private static Response writeResponse(Bridge bridge,
String path,
- InputStream inputStream) throws Exception {
-
- String returnMsg;
-
+ InputStream inputStream)
+ throws Exception {
// Open the output file
bridge.beginIteration();
-
long totalWritten = 0;
+ Exception ex = null;
// dataStream will close automatically in the end of the try.
// inputStream is closed by dataStream.close().
try (DataInputStream dataStream = new DataInputStream(inputStream)) {
while (bridge.setNext(dataStream)) {
++totalWritten;
}
- } catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
- } catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
- throw ex;
+ } catch (ClientAbortException cae) {
+ LOG.error("Remote connection closed by HAWQ", cae);
+ } catch (Exception e) {
+ LOG.error("Exception: totalWritten so far " + totalWritten + " to " + path, e);
+ ex = e;
} finally {
try {
bridge.endIteration();
} catch (Exception e) {
- // ignore ... any significant errors should already have been handled
+ if (ex == null)
+ ex = e;
--- End diff --
Another way would be to preserve throwing the exception inside original catch block (line 162), then here you would say
```
if (ex == null)
throw e;
else
throw ex;
```
and you would not need the block below (lines 170-172) as the original will still be thrown if endIterations() completes without an error.
---
[GitHub] incubator-hawq pull request #1334: Don't ignore exceptions during bridge.end...
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r164913643
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -159,20 +159,16 @@ private static Response writeResponse(Bridge bridge,
++totalWritten;
}
} catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
+ LOG.error("Remote connection closed by HAWQ", e);
} catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
+ LOG.error("totalWritten so far " + totalWritten + " to " + path);
throw ex;
} finally {
- try {
- bridge.endIteration();
- } catch (Exception e) {
- // ignore ... any significant errors should already have been handled
- }
+ bridge.endIteration();
--- End diff --
if Exception happened in try and is rethrown by catch and also endIteration throws an exception, then the original one will be forgotten and the latest one form endIteration will be thrown. If this is ok, we should at least log at error level the original one in catch block. Alternatively, have an error flag set by catch and then ignore exception here only if the flag is set to have the original one propagate to the client.
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r166099856
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -143,36 +142,38 @@ private static synchronized Response synchronizedWriteResponse(Bridge bridge,
private static Response writeResponse(Bridge bridge,
String path,
- InputStream inputStream) throws Exception {
-
- String returnMsg;
-
+ InputStream inputStream)
+ throws Exception {
// Open the output file
bridge.beginIteration();
-
long totalWritten = 0;
+ Exception ex = null;
// dataStream will close automatically in the end of the try.
// inputStream is closed by dataStream.close().
try (DataInputStream dataStream = new DataInputStream(inputStream)) {
while (bridge.setNext(dataStream)) {
++totalWritten;
}
- } catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
- } catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
- throw ex;
+ } catch (ClientAbortException cae) {
+ LOG.error("Remote connection closed by HAWQ", cae);
+ } catch (Exception e) {
+ LOG.error("Exception: totalWritten so far " + totalWritten + " to " + path, e);
+ ex = e;
} finally {
try {
bridge.endIteration();
} catch (Exception e) {
- // ignore ... any significant errors should already have been handled
+ if (ex == null)
+ ex = e;
}
+ // propagate any exceptions
+ if (ex != null)
+ throw ex;
}
String censuredPath = Utilities.maskNonPrintables(path);
- returnMsg = "wrote " + totalWritten + " bulks to " + censuredPath;
+ String returnMsg = "wrote " + totalWritten + " bulks to " + censuredPath;
--- End diff --
move string concat inside LOG.isDebugEnabled()
---
[GitHub] incubator-hawq pull request #1334: Don't ignore exceptions during bridge.end...
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r164928664
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -159,20 +159,16 @@ private static Response writeResponse(Bridge bridge,
++totalWritten;
}
} catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
+ LOG.error("Remote connection closed by HAWQ", e);
} catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
+ LOG.error("totalWritten so far " + totalWritten + " to " + path);
throw ex;
} finally {
- try {
- bridge.endIteration();
- } catch (Exception e) {
- // ignore ... any significant errors should already have been handled
- }
+ bridge.endIteration();
--- End diff --
I do not see exception being logged: `LOG.error("totalWritten so far " + totalWritten + " to " + path);` and only the latest one will be returned to client, not the original which potentially caused the issue
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by lavjain <gi...@git.apache.org>.
Github user lavjain commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r166122917
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -143,36 +142,38 @@ private static synchronized Response synchronizedWriteResponse(Bridge bridge,
private static Response writeResponse(Bridge bridge,
String path,
- InputStream inputStream) throws Exception {
-
- String returnMsg;
-
+ InputStream inputStream)
+ throws Exception {
// Open the output file
bridge.beginIteration();
-
long totalWritten = 0;
+ Exception ex = null;
// dataStream will close automatically in the end of the try.
// inputStream is closed by dataStream.close().
try (DataInputStream dataStream = new DataInputStream(inputStream)) {
while (bridge.setNext(dataStream)) {
++totalWritten;
}
- } catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
- } catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
- throw ex;
+ } catch (ClientAbortException cae) {
+ LOG.error("Remote connection closed by HAWQ", cae);
+ } catch (Exception e) {
+ LOG.error("Exception: totalWritten so far " + totalWritten + " to " + path, e);
+ ex = e;
} finally {
try {
bridge.endIteration();
} catch (Exception e) {
- // ignore ... any significant errors should already have been handled
+ if (ex == null)
+ ex = e;
}
+ // propagate any exceptions
+ if (ex != null)
+ throw ex;
}
String censuredPath = Utilities.maskNonPrintables(path);
- returnMsg = "wrote " + totalWritten + " bulks to " + censuredPath;
+ String returnMsg = "wrote " + totalWritten + " bulks to " + censuredPath;
--- End diff --
returnMsg is also being used in the response.
---
[GitHub] incubator-hawq issue #1334: Don't ignore exceptions during bridge.endIterati...
Posted by radarwave <gi...@git.apache.org>.
Github user radarwave commented on the issue:
https://github.com/apache/incubator-hawq/pull/1334
Any Jira number for this PR and commit? Thanks.
---
[GitHub] incubator-hawq pull request #1334: Don't ignore exceptions during bridge.end...
Posted by lavjain <gi...@git.apache.org>.
Github user lavjain commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r164928261
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -159,20 +159,16 @@ private static Response writeResponse(Bridge bridge,
++totalWritten;
}
} catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
+ LOG.error("Remote connection closed by HAWQ", e);
} catch (Exception ex) {
- LOG.debug("totalWritten so far " + totalWritten + " to " + path);
+ LOG.error("totalWritten so far " + totalWritten + " to " + path);
throw ex;
} finally {
- try {
- bridge.endIteration();
- } catch (Exception e) {
- // ignore ... any significant errors should already have been handled
- }
+ bridge.endIteration();
--- End diff --
Both (original catch exception as well endIteration) are logging at error level. Any exception would be propagated to the client.
---
[GitHub] incubator-hawq pull request #1334: HAWQ-1584. Don't ignore exceptions during...
Posted by lavjain <gi...@git.apache.org>.
GitHub user lavjain reopened a pull request:
https://github.com/apache/incubator-hawq/pull/1334
HAWQ-1584. Don't ignore exceptions during bridge.endIteration when writing to HDFS
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lavjain/incubator-hawq fix_writable
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/1334.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 #1334
----
commit b0e86b7d89f5087582ad89d405ebbba6d962241d
Author: lavjain <lj...@...>
Date: 2018-01-30T07:16:14Z
HAWQ-1584. Do not ignore exceptions during bridge.endIteration when writing to HDFS
----
---
[GitHub] incubator-hawq pull request #1334: Don't ignore exceptions during bridge.end...
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1334#discussion_r164912937
--- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/rest/WritableResource.java ---
@@ -159,20 +159,16 @@ private static Response writeResponse(Bridge bridge,
++totalWritten;
}
} catch (ClientAbortException e) {
- LOG.debug("Remote connection closed by HAWQ", e);
+ LOG.error("Remote connection closed by HAWQ", e);
--- End diff --
this was not considered an error, for some reason, we are not throwing exception in this case, so maybe warning level ?
---