You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by selvaganesang <gi...@git.apache.org> on 2018/04/17 05:40:06 UTC
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
GitHub user selvaganesang opened a pull request:
https://github.com/apache/trafodion/pull/1522
[TRAFODION-3009] Streamline error handling in Executor utility commands
Changes to avoid allocation of ComDiagsArea in some more places unless it is
needed to pass errors or warnings.
[TRAFODION-2917] Refactor Trafodion implementation of hdfs scan for text formatted hive tables
Disabling USE_LIBHDFS_SCAN by default. The new implementation of Hdfs Scan is now used to
scan the text type hive files.
Hdfs Scan is now stopped gracefully when the hive scan is cancelled. This avoids the random
core seen with new implementation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/trafodion trafodion-3007_5
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1522.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 #1522
----
commit 060bfc6c41277907af523d7bb62e02eb91b5bc86
Author: selvaganesang <se...@...>
Date: 2018-04-17T05:09:04Z
[TRAFODION-3009] Streamline error handling in Executor utility commands
Changes to avoid allocation of ComDiagsArea in some more places unless it is
needed to pass errors or warnings.
[TRAFODION-2917] Refactor Trafodion implementation of hdfs scan for text formatted hive tables
Disabling USE_LIBHDFS_SCAN by default. The new implementation of Hdfs Scan is now used to
scan the text type hive files.
Hdfs Scan is now stopped gracefully when the hive scan is cancelled. This avoids the random
core seen with new implementation.
----
---
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1522
---
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1522#discussion_r182136872
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -1119,7 +1119,7 @@ Lng32 ExeCliInterface::executeImmediate(const char * stmtStr,
{
// Allocate the diagnostics area if needed
// and populate the diagnostics conditions
- if (*globalDiags == NULL && retcode != 0) {
+ if (*globalDiags == NULL && retcode != 0 && retcode != 100) {
--- End diff --
I take it in the old code we would throw away the 100 warning somewhere else?
---
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1522#discussion_r182604952
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -626,14 +625,23 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
else
extraBytesRead_ = 0;
// headRoom_ is the number of extra bytes to be read (rangeTailIOSize)
- // If EOF is reached while reading the range and the extraBytes read
- // is less than headRoom_ then process all the data till EOF
- // TODO: If the whole range fits in one buffer, it is need too to process rows till EOF for the last range alone
- // No easy way to identify that last range read, but can identify that it is not the first range.
- // The rows could be read more than once if there are more than 2 ranges.
- // Fix optimizer not to have more than 2 ranges in that case
- if (retArray_[IS_EOF] && extraBytesRead_ < headRoom_ && hdfo->getStartOffset() != 0)
- extraBytesRead_ = 0;
+ // If the whole range fits in one buffer, it is needed to process rows till EOF for the last range alone.
+/*
--- End diff --
Is the commented-out code needed anymore? Should we get rid of it?
---
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1522#discussion_r182166447
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -1119,7 +1119,7 @@ Lng32 ExeCliInterface::executeImmediate(const char * stmtStr,
{
// Allocate the diagnostics area if needed
// and populate the diagnostics conditions
- if (*globalDiags == NULL && retcode != 0) {
+ if (*globalDiags == NULL && retcode != 0 && retcode != 100) {
--- End diff --
Returning retcode with a value of 100 is ok and should be treated as end of result by the callers. This additional check just avoids creating a ComDiagsArea if the return code is just 100.
---
[GitHub] trafodion pull request #1522: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1522#discussion_r182612555
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -626,14 +625,23 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
else
extraBytesRead_ = 0;
// headRoom_ is the number of extra bytes to be read (rangeTailIOSize)
- // If EOF is reached while reading the range and the extraBytes read
- // is less than headRoom_ then process all the data till EOF
- // TODO: If the whole range fits in one buffer, it is need too to process rows till EOF for the last range alone
- // No easy way to identify that last range read, but can identify that it is not the first range.
- // The rows could be read more than once if there are more than 2 ranges.
- // Fix optimizer not to have more than 2 ranges in that case
- if (retArray_[IS_EOF] && extraBytesRead_ < headRoom_ && hdfo->getStartOffset() != 0)
- extraBytesRead_ = 0;
+ // If the whole range fits in one buffer, it is needed to process rows till EOF for the last range alone.
+/*
--- End diff --
Yes. I can remove it in a later check-in
---