Mailing-List: contact commits-help@lucene.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@lucene.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1669313 - in /lucene/dev/trunk/solr: CHANGES.txt
 solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java
Date: Thu, 26 Mar 2015 10:42:23 -0000
To: commits@lucene.apache.org
From: romseygeek@apache.org
Message-Id: <20150326104223.51CE1AC006C@hades.apache.org>

Author: romseygeek
Date: Thu Mar 26 10:42:22 2015
New Revision: 1669313

URL: http://svn.apache.org/r1669313
Log:
SOLR-7203: Remove buggy no-op retries in HttpSolrClient

Modified:
    lucene/dev/trunk/solr/CHANGES.txt
    lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java

Modified: lucene/dev/trunk/solr/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/CHANGES.txt?rev=1669313&r1=1669312&r2=1669313&view=diff
==============================================================================
--- lucene/dev/trunk/solr/CHANGES.txt (original)
+++ lucene/dev/trunk/solr/CHANGES.txt Thu Mar 26 10:42:22 2015
@@ -405,6 +405,9 @@ Other Changes
 
 * SOLR-7291: Test indexing on ZK disconnect with ChaosMonkey tests (Ramkumar Aiyengar)
 
+* SOLR-7203: Remove buggy no-op retry code in HttpSolrClient (Alan Woodward,
+  Mark Miller, Greg Solovyev)
+
 ==================  5.0.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.

Modified: lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java?rev=1669313&r1=1669312&r2=1669313&view=diff
==============================================================================
--- lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java (original)
+++ lucene/dev/trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java Thu Mar 26 10:42:22 2015
@@ -21,7 +21,6 @@ import org.apache.http.Header;
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
 import org.apache.http.NameValuePair;
-import org.apache.http.NoHttpResponseException;
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.UrlEncodedFormEntity;
 import org.apache.http.client.methods.HttpEntityEnclosingRequestBase;
@@ -143,8 +142,6 @@ public class HttpSolrClient extends Solr
   
   private volatile boolean followRedirects = false;
   
-  private volatile int maxRetries = 0;
-  
   private volatile boolean useMultiPartPost;
   private final boolean internalClient;
 
@@ -297,8 +294,7 @@ public class HttpSolrClient extends Solr
   }
 
   protected HttpRequestBase createMethod(final SolrRequest request, String collection) throws IOException, SolrServerException {
-    HttpRequestBase method = null;
-    InputStream is = null;
+
     SolrParams params = request.getParams();
     Collection<ContentStream> streams = requestWriter.getContentStreams(request);
     String path = requestWriter.getPath(request);
@@ -325,158 +321,134 @@ public class HttpSolrClient extends Solr
     String basePath = baseUrl;
     if (collection != null)
       basePath += "/" + collection;
-    
-    int tries = maxRetries + 1;
-    try {
-      while( tries-- > 0 ) {
-        // Note: since we aren't do intermittent time keeping
-        // ourselves, the potential non-timeout latency could be as
-        // much as tries-times (plus scheduling effects) the given
-        // timeAllowed.
-        try {
-          if( SolrRequest.METHOD.GET == request.getMethod() ) {
-            if( streams != null ) {
-              throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "GET can't send streams!" );
-            }
-            method = new HttpGet(basePath + path + ClientUtils.toQueryString(wparams, false));
-          }
-          else if( SolrRequest.METHOD.POST == request.getMethod() || SolrRequest.METHOD.PUT == request.getMethod() ) {
 
-            String url = basePath + path;
-            boolean hasNullStreamName = false;
-            if (streams != null) {
-              for (ContentStream cs : streams) {
-                if (cs.getName() == null) {
-                  hasNullStreamName = true;
-                  break;
-                }
-              }
-            }
-            boolean isMultipart = ((this.useMultiPartPost && SolrRequest.METHOD.POST == request.getMethod())
-              || ( streams != null && streams.size() > 1 )) && !hasNullStreamName;
+    if (SolrRequest.METHOD.GET == request.getMethod()) {
+      if (streams != null) {
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "GET can't send streams!");
+      }
+      return new HttpGet(basePath + path + ClientUtils.toQueryString(wparams, false));
+    }
 
-            LinkedList<NameValuePair> postOrPutParams = new LinkedList<>();
-            if (streams == null || isMultipart) {
-              // send server list and request list as query string params
-              ModifiableSolrParams queryParams = calculateQueryParams(this.queryParams, wparams);
-              queryParams.add(calculateQueryParams(request.getQueryParams(), wparams));
-              String fullQueryUrl = url + ClientUtils.toQueryString( queryParams, false );
-              HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
-                new HttpPost(fullQueryUrl) : new HttpPut(fullQueryUrl);
-              if (!isMultipart) {
-                postOrPut.addHeader("Content-Type",
-                    "application/x-www-form-urlencoded; charset=UTF-8");
-              }
+    if (SolrRequest.METHOD.POST == request.getMethod() || SolrRequest.METHOD.PUT == request.getMethod()) {
 
-              List<FormBodyPart> parts = new LinkedList<>();
-              Iterator<String> iter = wparams.getParameterNamesIterator();
-              while (iter.hasNext()) {
-                String p = iter.next();
-                String[] vals = wparams.getParams(p);
-                if (vals != null) {
-                  for (String v : vals) {
-                    if (isMultipart) {
-                      parts.add(new FormBodyPart(p, new StringBody(v, StandardCharsets.UTF_8)));
-                    } else {
-                      postOrPutParams.add(new BasicNameValuePair(p, v));
-                    }
-                  }
-                }
-              }
+      String url = basePath + path;
+      boolean hasNullStreamName = false;
+      if (streams != null) {
+        for (ContentStream cs : streams) {
+          if (cs.getName() == null) {
+            hasNullStreamName = true;
+            break;
+          }
+        }
+      }
+      boolean isMultipart = ((this.useMultiPartPost && SolrRequest.METHOD.POST == request.getMethod())
+          || (streams != null && streams.size() > 1)) && !hasNullStreamName;
 
-              if (isMultipart && streams != null) {
-                for (ContentStream content : streams) {
-                  String contentType = content.getContentType();
-                  if(contentType==null) {
-                    contentType = BinaryResponseParser.BINARY_CONTENT_TYPE; // default
-                  }
-                  String name = content.getName();
-                  if(name==null) {
-                    name = "";
-                  }
-                  parts.add(new FormBodyPart(name, 
-                       new InputStreamBody(
-                           content.getStream(), 
-                           contentType, 
-                           content.getName())));
-                }
-              }
-              
-              if (parts.size() > 0) {
-                MultipartEntity entity = new MultipartEntity(HttpMultipartMode.STRICT);
-                for(FormBodyPart p: parts) {
-                  entity.addPart(p);
-                }
-                postOrPut.setEntity(entity);
-              } else {
-                //not using multipart
-                postOrPut.setEntity(new UrlEncodedFormEntity(postOrPutParams, StandardCharsets.UTF_8));
-              }
+      LinkedList<NameValuePair> postOrPutParams = new LinkedList<>();
+      if (streams == null || isMultipart) {
+        // send server list and request list as query string params
+        ModifiableSolrParams queryParams = calculateQueryParams(this.queryParams, wparams);
+        queryParams.add(calculateQueryParams(request.getQueryParams(), wparams));
+        String fullQueryUrl = url + ClientUtils.toQueryString(queryParams, false);
+        HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
+            new HttpPost(fullQueryUrl) : new HttpPut(fullQueryUrl);
+        if (!isMultipart) {
+          postOrPut.addHeader("Content-Type",
+              "application/x-www-form-urlencoded; charset=UTF-8");
+        }
 
-              method = postOrPut;
-            }
-            // It is has one stream, it is the post body, put the params in the URL
-            else {
-              String pstr = ClientUtils.toQueryString(wparams, false);
-              HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
-                new HttpPost(url + pstr) : new HttpPut(url + pstr);
-
-              // Single stream as body
-              // Using a loop just to get the first one
-              final ContentStream[] contentStream = new ContentStream[1];
-              for (ContentStream content : streams) {
-                contentStream[0] = content;
-                break;
-              }
-              if (contentStream[0] instanceof RequestWriter.LazyContentStream) {
-                postOrPut.setEntity(new InputStreamEntity(contentStream[0].getStream(), -1) {
-                  @Override
-                  public Header getContentType() {
-                    return new BasicHeader("Content-Type", contentStream[0].getContentType());
-                  }
-                  
-                  @Override
-                  public boolean isRepeatable() {
-                    return false;
-                  }
-                  
-                });
+        List<FormBodyPart> parts = new LinkedList<>();
+        Iterator<String> iter = wparams.getParameterNamesIterator();
+        while (iter.hasNext()) {
+          String p = iter.next();
+          String[] vals = wparams.getParams(p);
+          if (vals != null) {
+            for (String v : vals) {
+              if (isMultipart) {
+                parts.add(new FormBodyPart(p, new StringBody(v, StandardCharsets.UTF_8)));
               } else {
-                postOrPut.setEntity(new InputStreamEntity(contentStream[0].getStream(), -1) {
-                  @Override
-                  public Header getContentType() {
-                    return new BasicHeader("Content-Type", contentStream[0].getContentType());
-                  }
-                  
-                  @Override
-                  public boolean isRepeatable() {
-                    return false;
-                  }
-                });
+                postOrPutParams.add(new BasicNameValuePair(p, v));
               }
-              method = postOrPut;
             }
           }
-          else {
-            throw new SolrServerException("Unsupported method: "+request.getMethod() );
-          }
         }
-        catch( NoHttpResponseException r ) {
-          method = null;
-          if(is != null) {
-            is.close();
-          }
-          // If out of tries then just rethrow (as normal error).
-          if (tries < 1) {
-            throw r;
+
+        if (isMultipart && streams != null) {
+          for (ContentStream content : streams) {
+            String contentType = content.getContentType();
+            if (contentType == null) {
+              contentType = BinaryResponseParser.BINARY_CONTENT_TYPE; // default
+            }
+            String name = content.getName();
+            if (name == null) {
+              name = "";
+            }
+            parts.add(new FormBodyPart(name,
+                new InputStreamBody(
+                    content.getStream(),
+                    contentType,
+                    content.getName())));
           }
         }
+
+        if (parts.size() > 0) {
+          MultipartEntity entity = new MultipartEntity(HttpMultipartMode.STRICT);
+          for (FormBodyPart p : parts) {
+            entity.addPart(p);
+          }
+          postOrPut.setEntity(entity);
+        } else {
+          //not using multipart
+          postOrPut.setEntity(new UrlEncodedFormEntity(postOrPutParams, StandardCharsets.UTF_8));
+        }
+
+        return postOrPut;
+      }
+      // It is has one stream, it is the post body, put the params in the URL
+      else {
+        String pstr = ClientUtils.toQueryString(wparams, false);
+        HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
+            new HttpPost(url + pstr) : new HttpPut(url + pstr);
+
+        // Single stream as body
+        // Using a loop just to get the first one
+        final ContentStream[] contentStream = new ContentStream[1];
+        for (ContentStream content : streams) {
+          contentStream[0] = content;
+          break;
+        }
+        if (contentStream[0] instanceof RequestWriter.LazyContentStream) {
+          postOrPut.setEntity(new InputStreamEntity(contentStream[0].getStream(), -1) {
+            @Override
+            public Header getContentType() {
+              return new BasicHeader("Content-Type", contentStream[0].getContentType());
+            }
+
+            @Override
+            public boolean isRepeatable() {
+              return false;
+            }
+
+          });
+        } else {
+          postOrPut.setEntity(new InputStreamEntity(contentStream[0].getStream(), -1) {
+            @Override
+            public Header getContentType() {
+              return new BasicHeader("Content-Type", contentStream[0].getContentType());
+            }
+
+            @Override
+            public boolean isRepeatable() {
+              return false;
+            }
+          });
+        }
+        return postOrPut;
       }
-    } catch (IOException ex) {
-      throw new SolrServerException("error reading streams", ex);
     }
-    
-    return method;
+
+    throw new SolrServerException("Unsupported method: " + request.getMethod());
+
   }
   
   protected NamedList<Object> executeMethod(HttpRequestBase method, final ResponseParser processor) throws SolrServerException {
@@ -706,21 +678,9 @@ public class HttpSolrClient extends Solr
   }
   
   /**
-   * Set maximum number of retries to attempt in the event of transient errors.
-   * <p>
-   * Maximum number of retries to attempt in the event of transient errors.
-   * Default: 0 (no) retries. No more than 1 recommended.
-   * </p>
-   * @param maxRetries
-   *          No more than 1 recommended
+   * @deprecated retries should be implemented in client code, and should be considered carefully per-request
    */
-  public void setMaxRetries(int maxRetries) {
-    if (maxRetries > 1) {
-      log.warn("HttpSolrServer: maximum Retries " + maxRetries
-          + " > 1. Maximum recommended retries is 1.");
-    }
-    this.maxRetries = maxRetries;
-  }
+  public void setMaxRetries(int maxRetries) { }
   
   public void setRequestWriter(RequestWriter requestWriter) {
     this.requestWriter = requestWriter;