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 2017/01/19 02:06:56 UTC
[GitHub] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
GitHub user selvaganesang opened a pull request:
https://github.com/apache/incubator-trafodion/pull/918
[TRAFODION-2420] RMS Enhancements
Added yet another offender feature to list query ids that has a total
IO time for any storage engine opertor consuming longer than a given
number of seconds.
./offender -s se_offender
Will list the query ids along with the table name. SEE
$TRAF_HOME/export/limited-support-tools/LSO/README
The "Number of SQL Processes" counter is now made multi-fragment aware
and hence contains the actual number of ESPs used + 1 for master
process.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/incubator-trafodion se_offender_1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/918.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 #918
----
commit 45ff9540eeeacb46e4e65e2b6a57b01322223760
Author: selvaganesang <se...@esgyn.com>
Date: 2017-01-19T01:58:56Z
[TRAFODION-2420] RMS Enhancements
Added yet another offender feature to list query ids that has a total
IO time for any storage engine opertor consuming longer than a given
number of seconds.
./offender -s se_offender
Will list the query ids along with the table name. SEE
$TRAF_HOME/export/limited-support-tools/LSO/README
The "Number of SQL Processes" counter is now made multi-fragment aware
and hence contains the actual number of ESPs used + 1 for master
process.
----
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by nonstop-qfchen <gi...@git.apache.org>.
Github user nonstop-qfchen commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97324910
--- Diff: core/sql/executor/ExStats.h ---
@@ -3791,9 +3791,9 @@ NA_EIDPROC
short &cmpPriority() {return cmpPriority_;}
short &dp2Priority() {return dp2Priority_;}
short &fixupPriority() {return fixupPriority_;}
- inline void setNumSqlProcs(short i) {numSqlProcs_ = i; }
+ void incNumEspsInUse() { numOfTotalEspsUsed_++; }
inline void setNumCpus(short i) {numCpus_ = i; }
- inline short getNumSqlProcs() { return numSqlProcs_; }
+ inline short getNumSqlProcs() { return numOfTotalEspsUsed_+1; }
--- End diff --
Should we +1 every time we call getNumSqlProcs()?
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by nonstop-qfchen <gi...@git.apache.org>.
Github user nonstop-qfchen commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97323697
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2741,13 +2749,21 @@ Int64 ExHbaseAccessStats::getNumVal(Int32 i) const
NABoolean ExHbaseAccessStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
{
- blockTime_ = timer_.filterForSEstats(currTimespec);
- if (blockTime_ >= filter)
- return TRUE;
+ Int64 sumIOTime;
--- End diff --
The same logic seems duplicated between this class and ExHdfsScanStats. Maybe some refactoring?
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97343889
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2741,13 +2749,21 @@ Int64 ExHbaseAccessStats::getNumVal(Int32 i) const
NABoolean ExHbaseAccessStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
{
- blockTime_ = timer_.filterForSEstats(currTimespec);
- if (blockTime_ >= filter)
- return TRUE;
+ Int64 sumIOTime;
--- End diff --
Will take care of your suggestion, when this is 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97344477
--- Diff: core/sql/executor/ExStats.h ---
@@ -3791,9 +3791,9 @@ NA_EIDPROC
short &cmpPriority() {return cmpPriority_;}
short &dp2Priority() {return dp2Priority_;}
short &fixupPriority() {return fixupPriority_;}
- inline void setNumSqlProcs(short i) {numSqlProcs_ = i; }
+ void incNumEspsInUse() { numOfTotalEspsUsed_++; }
inline void setNumCpus(short i) {numCpus_ = i; }
- inline short getNumSqlProcs() { return numSqlProcs_; }
+ inline short getNumSqlProcs() { return numOfTotalEspsUsed_+1; }
--- End diff --
Yes. Because if the query had just master, then the number of SQL procs will be zero. Only the numOfTotalEspsUsed_ 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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by nonstop-qfchen <gi...@git.apache.org>.
Github user nonstop-qfchen commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97323080
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2357,11 +2357,19 @@ Int64 ExHdfsScanStats::getNumVal(Int32 i) const
NABoolean ExHdfsScanStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
{
- blockTime_ = timer_.filterForSEstats(currTimespec);
- if (blockTime_ >= filter)
- return TRUE;
+ Int64 sumIOTime;
+
+ if (filter > 0) {
+ blockTime_ = timer_.filterForSEstats(currTimespec);
+ if (blockTime_ >= filter)
+ return TRUE;
+ }
else
- return FALSE;
+ if (queryId_ != NULL && (sumIOTime = timer_.getTime()) > 0 && (sumIOTime = sumIOTime /(1000000LL)) >= -filter) {
--- End diff --
Could write as sumIOTime /= 1000000LL
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/918
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by nonstop-qfchen <gi...@git.apache.org>.
Github user nonstop-qfchen commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97324599
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2357,11 +2357,19 @@ Int64 ExHdfsScanStats::getNumVal(Int32 i) const
NABoolean ExHdfsScanStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
--- End diff --
I wonder if the type of filter argument should be Int64 here, since Int64 / 1000000LL could be a value larger than max(Int32).
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97414406
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2357,11 +2357,19 @@ Int64 ExHdfsScanStats::getNumVal(Int32 i) const
NABoolean ExHdfsScanStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
--- End diff --
The filter is in seconds and comes from passing the user command. The user command is something like
select * from table(statistics(null, 'SE_OFFENDER=<t1_in_secs>)). The parsing of the second parameter should take care of this.
The blockTime_ is currently in 32bit, because we don't expect the query to be blocked so long. It should timeout much earlier than this from hbase.
---
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] incubator-trafodion pull request #918: [TRAFODION-2420] RMS Enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/918#discussion_r97343740
--- Diff: core/sql/executor/ExStats.cpp ---
@@ -2741,13 +2749,21 @@ Int64 ExHbaseAccessStats::getNumVal(Int32 i) const
NABoolean ExHbaseAccessStats::filterForSEstats(struct timespec currTimespec, Lng32 filter)
{
- blockTime_ = timer_.filterForSEstats(currTimespec);
- if (blockTime_ >= filter)
- return TRUE;
+ Int64 sumIOTime;
--- End diff --
Yes. Can be done. I am thinking of getting rid of one of these classes because both have same member variables sometime in future.
---
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.
---