gerlowskija commented on code in PR #2714:
URL: https://github.com/apache/solr/pull/2714#discussion_r1763347010


##########
solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java:
##########
@@ -102,6 +111,24 @@ public static String buildRequestUrl(
     return basePath + path;
   }
 
+  /**
+   * Syntactic-sugar to set/unset {@link ClientUtils#baseUrlOverride} via 
try-with-resources blocks
+   *
+   * <p>Instances should not be used across threads, and <em>must</em> be 
closed by the same thread
+   * that opened them
+   */
+  public static class BaseUrlOverride implements Closeable {
+    public BaseUrlOverride(String baseUrl) {
+      baseUrlOverride.set(baseUrl);
+    }
+
+    public void close() {
+      baseUrlOverride.remove();
+    }
+  }
+
+  public static final ThreadLocal<String> baseUrlOverride = new 
ThreadLocal<>();

Review Comment:
   > ThreadLocal's have their use to communicate something across API layers 
that can't or shouldn't include the extra information.
   
   Maybe I'm misreading your comment, but are you suggesting that this POC 
differs from the valid ThreadLocal use-case you described?
   
   Here we have API layers that we don't want to be (explicitly) concerned with 
the baseURL - namely SolrRequest, and SolrClient's request-y methods.  So we 
use the lambda/thread-local combo to tunnel the information down to where the 
path building occurs.  Which feels to me very much like the valid use case of 
ThreadLocal's you described above?
   
   Or maybe you're saying that this POC uses ThreadLocals in a valid/useful 
way, it's just not necessary because other options are available?
   
   > I don't recall seeing an API embrace a ThreadLocal to accomplish something 
at it's front door / prominently.
   
   This variable can (and should) be private.  That's a PoC/code-cleanup 
mistake on my part that's easily remedied.
   
   But beyond that, the thread-local **isn't** in the API anywhere - not in the 
nested `BaseUrlOverride` class, not in `SolrClient`, not even of any method in 
`ClientUtils`.  Can you elaborate a bit on how the API is "embracing" the 
thread-local prominently?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org

Reply via email to