Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley merged PR #2927: URL: https://github.com/apache/solr/pull/2927 -- 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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1915917903
##
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##
@@ -1170,6 +1171,21 @@ protected Object _getHandler() {
return handler;
}
+ /** Gets the client (user-agent) SolrJ version, or null if isn't SolrJ. */
+ public SolrVersion getUserAgentSolrVersion() {
+String header = req.getHeader("User-Agent");
+if (header == null || !header.startsWith("Solr")) {
+ return null;
+}
+try {
+ return SolrVersion.valueOf(header.substring(header.lastIndexOf(' ') +
1));
Review Comment:
I added this to the docs.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1915918653
##
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##
@@ -1170,6 +1171,21 @@ protected Object _getHandler() {
return handler;
}
+ /** Gets the client (user-agent) SolrJ version, or null if isn't SolrJ. */
+ public SolrVersion getUserAgentSolrVersion() {
+String header = req.getHeader("User-Agent");
+if (header == null || !header.startsWith("Solr")) {
+ return null;
+}
+try {
+ return SolrVersion.valueOf(header.substring(header.lastIndexOf(' ') +
1));
Review Comment:
BTW that method has no callers; isn't tested. I did manually test it.
Obviously I intend to have it be used soon.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on PR #2927: URL: https://github.com/apache/solr/pull/2927#issuecomment-2591610680 Ready to merge; will do within a couple days if I hear nothing. -- 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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1914069901
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##
@@ -73,7 +74,7 @@ public class HttpJdkSolrClient extends HttpSolrClientBase {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private static final String USER_AGENT =
- "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] 1.0";
+ "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] " +
SolrVersion.LATEST_STRING;
Review Comment:
Yes.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1914062827
##
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##
@@ -1170,6 +1171,21 @@ protected Object _getHandler() {
return handler;
}
+ /** Gets the client (user-agent) SolrJ version, or null if isn't SolrJ. */
+ public SolrVersion getUserAgentSolrVersion() {
+String header = req.getHeader("User-Agent");
+if (header == null || !header.startsWith("Solr")) {
+ return null;
+}
+try {
+ return SolrVersion.valueOf(header.substring(header.lastIndexOf(' ') +
1));
Review Comment:
Yeah; I thought it would have no consequence so I did nothing about it. At
least by returning a SolrVersion instead of null, we communicate indirectly
that it's at least SolrJ. I could add a javadoc comment on 1.0 and 2.0 being
used prior to 9.8.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
madrob commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1913952760
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##
@@ -73,7 +74,7 @@ public class HttpJdkSolrClient extends HttpSolrClientBase {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private static final String USER_AGENT =
- "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] 1.0";
+ "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] " +
SolrVersion.LATEST_STRING;
Review Comment:
It's ok to lose the distinction in version number because we still have v1
and v2 clients in the class names?
##
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##
@@ -1170,6 +1171,21 @@ protected Object _getHandler() {
return handler;
}
+ /** Gets the client (user-agent) SolrJ version, or null if isn't SolrJ. */
+ public SolrVersion getUserAgentSolrVersion() {
+String header = req.getHeader("User-Agent");
+if (header == null || !header.startsWith("Solr")) {
+ return null;
+}
+try {
+ return SolrVersion.valueOf(header.substring(header.lastIndexOf(' ') +
1));
Review Comment:
If this gets an older client it will return 1.0 or 2.0, which can be
somewhat misleading.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
dsmiley commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1913233926
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -106,11 +106,15 @@
*
*/
public class Http2SolrClient extends HttpSolrClientBase {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String REQ_PRINCIPAL_KEY = "solr-req-principal";
+ private static final String USER_AGENT =
+ "Solr["
Review Comment:
I'd hate to create constants merely for the parts of this string that we
concatenate -- I hate that coding practice, to be blunt. I could see a utility
method that composes them. But any way it's tested so I'm not sure what the
risk/concern of _not_ doing what you suggest is. This is a one-liner BTW.
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
jdyer1 commented on code in PR #2927:
URL: https://github.com/apache/solr/pull/2927#discussion_r1909541445
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -106,11 +106,15 @@
*
*/
public class Http2SolrClient extends HttpSolrClientBase {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String REQ_PRINCIPAL_KEY = "solr-req-principal";
+ private static final String USER_AGENT =
+ "Solr["
Review Comment:
Could this go in the superclass so it doesn't have to exist twice?
--
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
Re: [PR] SOLR-17611 SolrJ User-Agent, pass Solr version [solr]
jdyer1 commented on PR #2927: URL: https://github.com/apache/solr/pull/2927#issuecomment-2581363581 There are asserts in the unit tests for the user-agent, so changing it will cause failures I think. See `HttpSolrClientTestBase#expectedUserAgent`. If we put the specification version in a `protected static` variable, the unit test can easily override it, which might be adequate enough of a unit testing strategy? I was looking around and maybe ByteBuddy's `PackageDefinitionStrategy` could be used to mock this? Even if possible (I have no idea) I guess that would be more effort than it is worth! I agree something in the smoke tester would be the best way to verify correctness. -- 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
