jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: svn commit: r1491213 - in /jmeter/trunk: src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/ src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/ xdocs/
Date Sun, 09 Jun 2013 13:40:39 GMT
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.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message