[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1574: SOLR-14566: Add request-ID to all distrib-search requests

[GitBox <gi...@apache.org>]

gerlowskija commented on a change in pull request #1574:
URL: https://github.com/apache/lucene-solr/pull/1574#discussion_r444844913



##########
File path: solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
##########
@@ -500,6 +509,29 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
     }
   }
 
+  private void tagRequestWithRequestId(ResponseBuilder rb) {
+    String rid = getRequestId(rb.req);
+    if (StringUtils.isBlank(rb.req.getParams().get(CommonParams.REQUEST_ID))) {
+      ModifiableSolrParams params = new ModifiableSolrParams(rb.req.getParams());
+      params.add(CommonParams.REQUEST_ID, rid);//add rid to the request so that shards see it
+      rb.req.setParams(params);
+    }
+    if (rb.isDistrib) {
+      rb.rsp.addToLog(CommonParams.REQUEST_ID, rid); //to see it in the logs of the landing core
+    }
+  }
+
+  public static String getRequestId(SolrQueryRequest req) {
+    String rid = req.getParams().get(CommonParams.REQUEST_ID);
+    return StringUtils.isNotBlank(rid) ? rid : generateRid(req);
+  }
+
+  @SuppressForbidden(reason = "Need currentTimeMillis, only used for naming")
+  private static String generateRid(SolrQueryRequest req) {
+    String hostName = req.getCore().getCoreContainer().getHostName();
+    return hostName + "-" + req.getCore().getName() + "-" + System.currentTimeMillis() + "-" + ridCounter.getAndIncrement();

Review comment:
       I'm a bit torn.
   
   I agree that the length makes this a bit "in-your-face", and that a hash would be a good way to condense things.  I don't love the verbose-ness as things are now.  But, technically speaking, that loses us our uniqueness guarantee.  And if we're not shooting for strict-uniqueness here, then aren't we just as good with the NOW timestamp that was originally proposed but set aside for lack of uniqueness?
   
   To zoom out, we've got three options for what the identifier is here:
   
   1. **NOW timestamp** - concise with some semantic meaning, but not guaranteed to be unique
   2. **RequestID** - very semantically meaningful and guaranteed to be unique, but "in your face"
   3. **Hashed RequestID** - concise, but no semantic meaning and not guaranteed to be unique (though slightly more unique than a NOW timestamp?)
   
   I originally implemented (1), and then moved to (2) based on feedback in JIRA.  But if there's no consensus there then maybe I'll go back to (1).  In any case I don't see much advantage in (3) over (1)




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org