Hello,
Fixed thanks for review.
Regards
Philippe
On Sun, Jun 9, 2013 at 3:30 PM, sebb <sebbaz@gmail.com> wrote:
> On 9 June 2013 14:07, <pmouawad@apache.org> wrote:
> > Author: pmouawad
> > Date: Sun Jun 9 13:07:32 2013
> > New Revision: 1491213
> >
> > URL: http://svn.apache.org/r1491213
> > Log:
> > Bug 55084 - Add timeout support for JDBC Request
> > Bugzilla Id: 55084
>
> There are some problems with this patch as detailed below.
>
> Please fix ASAP.
>
> > Modified:
> >
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java
> >
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java
> >
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties
> >
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties
> > jmeter/trunk/xdocs/changes.xml
> >
> > Modified:
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java?rev=1491213&r1=1491212&r2=1491213&view=diff
> >
> ==============================================================================
> > ---
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java
> (original)
> > +++
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java
> Sun Jun 9 13:07:32 2013
> > @@ -35,6 +35,7 @@ import java.util.List;
> > import java.util.Map;
> > import java.util.concurrent.ConcurrentHashMap;
> >
> > +import org.apache.commons.lang3.StringUtils;
> > import org.apache.jmeter.samplers.SampleResult;
> > import org.apache.jmeter.save.CSVSaveService;
> > import org.apache.jmeter.testelement.AbstractTestElement;
> > @@ -111,7 +112,8 @@ public abstract class AbstractJDBCTestEl
> > private String queryArguments = ""; // $NON-NLS-1$
> > private String queryArgumentsTypes = ""; // $NON-NLS-1$
> > private String variableNames = ""; // $NON-NLS-1$
> > - private String resultVariable = "";
> > + private String resultVariable = ""; // $NON-NLS-1$
> > + private String queryTimeout = ""; // $NON-NLS-1$
> >
> > /**
> > * Cache of PreparedStatements stored in a per-connection basis.
> Each entry of this
> > @@ -142,6 +144,7 @@ public abstract class AbstractJDBCTestEl
> > String _queryType = getQueryType();
> > if (SELECT.equals(_queryType)) {
> > stmt = conn.createStatement();
> > + stmt.setQueryTimeout(getIntegerQueryTimeout());
> > ResultSet rs = null;
> > try {
> > rs = stmt.executeQuery(getQuery());
> > @@ -159,6 +162,7 @@ public abstract class AbstractJDBCTestEl
> > return sb.getBytes(ENCODING);
> > } else if (UPDATE.equals(_queryType)) {
> > stmt = conn.createStatement();
> > + stmt.setQueryTimeout(getIntegerQueryTimeout());
> > stmt.executeUpdate(getQuery());
> > int updateCount = stmt.getUpdateCount();
> > String results = updateCount + " updates";
> > @@ -332,9 +336,15 @@ public abstract class AbstractJDBCTestEl
> > } else {
> > pstmt = conn.prepareStatement(getQuery());
> > }
> > + pstmt.setQueryTimeout(getIntegerQueryTimeout());
> > // PreparedStatementMap is associated to one connection so
> > // 2 threads cannot use the same PreparedStatement map at
> the same time
> > preparedStatementMap.put(getQuery(), pstmt);
> > + } else {
> > + int timeoutInS = getIntegerQueryTimeout();
> > + if(pstmt.getQueryTimeout() != timeoutInS) {
> > + pstmt.setQueryTimeout(getIntegerQueryTimeout());
> > + }
> > }
> > pstmt.clearParameters();
> > return pstmt;
> > @@ -457,6 +467,35 @@ public abstract class AbstractJDBCTestEl
> > } catch (SQLException e) {
> > log.warn("Error closing ResultSet", e);
> > }
> > + }
> > +
> > + /**
> > + * @return the integer representation queryTimeout
> > + */
> > + public int getIntegerQueryTimeout() {
> > + int timeout = 0;
> > + try {
> > + if(StringUtils.isNumeric(query)) {
>
> Not sure what the point of the numeric check is - why not just let the
> parseInt fail?
>
To avoid Exception throwing which has higher cost than testing for numeric.
Feel free to change if you want.
> And why check "query" and then parse "queryTimeout"?
>
Good catch
>
> This check does not appear in the original patch.
>
> > + timeout = Integer.parseInt(queryTimeout);
> > + }
> > + } catch (NumberFormatException nfe) {
> > + timeout = 0;
> > + }
> > + return timeout;
> > + }
> > +
> > + /**
> > + * @return the queryTimeout
> > + */
> > + public String getQueryTimeout() {
> > + return queryTimeout ;
> > + }
> > +
> > + /**
> > + * @param resultVariable the variable name in which results will be
> stored
>
> Javadoc is wrong.
>
Fixed
>
> > + */
> > + public void setQueryTimeout(String queryTimeout) {
> > + this.queryTimeout = queryTimeout;
> > }
> >
> > public String getQuery() {
> >
> > Modified:
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java?rev=1491213&r1=1491212&r2=1491213&view=diff
> >
> ==============================================================================
> > ---
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java
> (original)
> > +++
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java
> Sun Jun 9 13:07:32 2013
> > @@ -42,28 +42,33 @@ public abstract class JDBCTestElementBea
> > "queryArgumentsTypes", // $NON-NLS-1$
> > "variableNames", // $NON-NLS-1$
> > "resultVariable", // $NON-NLS-1$
> > + "queryTimeout" // $NON-NLS-1$
> > });
> >
> > PropertyDescriptor p = property("dataSource"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> >
> > p = property("queryArguments"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> >
> > p = property("queryArgumentsTypes"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> >
> > p = property("variableNames"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> >
> > p = property("resultVariable"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> >
> > + p = property("queryTimeout"); // $NON-NLS-1$
> > + p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > + p.setValue(DEFAULT, "");
> > +
> > p = property("queryType"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > p.setValue(DEFAULT, AbstractJDBCTestElement.SELECT);
> > @@ -82,7 +87,7 @@ public abstract class JDBCTestElementBea
> >
> > p = property("query"); // $NON-NLS-1$
> > p.setValue(NOT_UNDEFINED, Boolean.TRUE);
> > - p.setValue(DEFAULT, "");
> > + p.setValue(DEFAULT, ""); // $NON-NLS-1$
> > p.setPropertyEditorClass(TextAreaEditor.class);
> >
> > }
> >
> > Modified:
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties?rev=1491213&r1=1491212&r2=1491213&view=diff
> >
> ==============================================================================
> > ---
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties
> (original)
> > +++
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties
> Sun Jun 9 13:07:32 2013
> > @@ -30,4 +30,5 @@ variableNames.displayName=Variable names
> > variableNames.shortDescription=Output variable names for each column
> (comma separated)
> > resultVariable.displayName=Result variable name
> > resultVariable.shortDescription=Name of the JMeter variable that stores
> the result set objects in a list of maps for looking up results by column
> name.
> > -
> > +queryTimeout.displayName=Query timeout
> > +queryTimeout.shortDescription=The timeout of statement measured in
> seconds
> > \ No newline at end of file
> >
> > Modified:
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties?rev=1491213&r1=1491212&r2=1491213&view=diff
> >
> ==============================================================================
> > ---
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties
> (original)
> > +++
> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties
> Sun Jun 9 13:07:32 2013
> > @@ -31,3 +31,5 @@ sql.displayName=Requ\u00EAte SQL
> > varName.displayName=Nom de liaison avec le pool
> > variableNames.displayName=Noms des variables
> > variableNames.shortDescription=Noms des variables en sortie pour chaque
> colonne (s\u00E9par\u00E9s par des virgules)
> > +queryTimeout.displayName=Timeout de la requ\u00EAte
> > +queryTimeout.shortDescription=Timeout de le requ\u00EAte en secondes
> > \ No newline at end of file
> >
> > Modified: jmeter/trunk/xdocs/changes.xml
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1491213&r1=1491212&r2=1491213&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/xdocs/changes.xml (original)
> > +++ jmeter/trunk/xdocs/changes.xml Sun Jun 9 13:07:32 2013
> > @@ -174,6 +174,7 @@ Transaction Controller now sets Response
> > <li><bugzilla>54798</bugzilla> - Using subject from EML-file
for SMTP
> Sampler</li>
> > <li><bugzilla>54759</bugzilla> - SSLPeerUnverifiedException using
HTTPS
> , property documented</li>
> > <li><bugzilla>54896</bugzilla> - JUnit sampler gives only “failed
to
> create an instance of the class†message with constructor problems</li>
> > +<li><bugzilla>55084</bugzilla> - Add timeout support for JDBC
> Request</li>
> > </ul>
> >
> > <h3>Controllers</h3>
> >
> >
>
--
Cordialement.
Philippe Mouawad.
|