[PR] Update taking-solr-to-production.adoc environment variable SOLR_ENV to SOLR_INCLUDE [solr]
bkysela opened a new pull request, #3252: URL: https://github.com/apache/solr/pull/3252 Solr 9.8 start script `bin/solr` reads `SOLR_INCLUDE` environment variable not `SOLR_ENV`. -- 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
[PR] DefaultPackageRepository: simplify HTTP & JSON [solr]
dsmiley opened a new pull request, #3253: URL: https://github.com/apache/solr/pull/3253 The use of a SolrClient didn't seem to make sense if the web server isn't (necessarily) Solr. Seemed to be getting more in the way. There was some double or even triple(?) JSON back & forth parsing / serializing that I couldn't let be on principle. Ran tests, including integration tests. Didn't check if this actually uses GitHub. Not sure if we lose some Solr-to-Solr auth that's relevant/important. If so, I could switch to Jetty HttpClient but wouldn't love that. -- 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-17682: QueryResponseWriter hierarchy refactor [solr]
dsmiley commented on PR #3209: URL: https://github.com/apache/solr/pull/3209#issuecomment-2712803895 Planning to merge in a couple days without further feedback -- 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] SimpleOrderedMap MapWriter constructor [solr]
dsmiley merged PR #3235: URL: https://github.com/apache/solr/pull/3235 -- 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] SolrJ ResponseParser API improvements, minor [solr]
dsmiley commented on code in PR #3248:
URL: https://github.com/apache/solr/pull/3248#discussion_r1987294113
##
solr/solrj/src/java/org/apache/solr/client/solrj/ResponseParser.java:
##
@@ -16,45 +16,31 @@
*/
package org.apache.solr.client.solrj;
+import java.io.IOException;
import java.io.InputStream;
-import java.io.Reader;
import java.util.Collection;
-import java.util.Set;
import org.apache.solr.common.util.NamedList;
/**
+ * SolrJ Solr response parser.
+ *
* @since solr 1.3
*/
public abstract class ResponseParser {
- public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList processResponse(InputStream body, String
encoding);
+ /** The writer type placed onto the request as the {@code wt} param. */
+ public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList processResponse(Reader reader);
-
- /**
- * A well-behaved ResponseParser will return its content-type.
- *
- * @return the content-type this parser expects to parse
- * @deprecated use {@link #getContentTypes()} instead
- */
- @Deprecated
- public String getContentType() {
-return null;
- }
+ public abstract NamedList processResponse(InputStream body, String
encoding)
+ throws IOException;
/**
* A well-behaved ResponseParser will return the content-types it supports.
*
- * @return the content-type values that this parser is capable of parsing.
+ * @return the content-type values that this parser is capable of parsing.
Never null. Empty means
+ * no enforcement.
Review Comment:
I could easily imagine a misalignment between the response from Solr and the
parser (different format). This acts as a sanity check instead of blindly
parsing what we could rule out with a friendly error message.
--
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] SolrJ ResponseParser API improvements, minor [solr]
dsmiley commented on code in PR #3248:
URL: https://github.com/apache/solr/pull/3248#discussion_r1987289810
##
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##
@@ -100,18 +100,15 @@ public String getWriterType() {
}
@Override
- public NamedList processResponse(InputStream body, String
encoding) {
-try {
- if (body.read() >= 0) readFile.set(true);
-} catch (IOException e) {
- throw new RuntimeException(e);
-}
+ public NamedList processResponse(InputStream body, String
encoding)
+ throws IOException {
+if (body.read() >= 0) readFile.set(true);
return null;
}
@Override
- public NamedList processResponse(Reader reader) {
-throw new UnsupportedOperationException("TODO unimplemented"); //
TODO
+ public Set getContentTypes() {
+return Set.of(); // don't enforce
Review Comment:
The file diff is confusing here because what was removed and added have
nothing to do with each other. Two things are happening here:
(1) removed processResponse(Reader) from ResponseParser and thus the
implementation in this file need not implement (it was compelled to before)
(2) getContentTypes is now abstract and so all implementations must implement
--
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
[jira] [Resolved] (SOLR-17518) Refactor out a XmlRequestWriter so that RequestWriter is abstract
[ https://issues.apache.org/jira/browse/SOLR-17518?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Pierre Salagnac resolved SOLR-17518. Fix Version/s: 9.9 Resolution: Fixed > Refactor out a XmlRequestWriter so that RequestWriter is abstract > - > > Key: SOLR-17518 > URL: https://issues.apache.org/jira/browse/SOLR-17518 > Project: Solr > Issue Type: Task > Components: SolrJ >Reporter: David Smiley >Assignee: Pierre Salagnac >Priority: Minor > Labels: newdev, pull-request-available > Fix For: main (10.0), 9.9 > > Time Spent: 2h 20m > Remaining Estimate: 0h > > RequestWriter writes XML; some subclasses write other things. This is > terrible API design; the XML choice should be a subclass, RequestWriter > should be abstract (or an interface). > While we're at this, the XML generation is kind of split into multiple > places; it should be consolidated: UpdateRequest & ClientUtils have XML > generation methods. Those methods should move to the new XmlRequestWriter. > BinaryRequestWriter should probably be final and/or ensure the CBOR one does > not subclass it, which is weird since CBOR != "javabin". > Be sure to note this refactoring in the update notes since SolrJ users will > be impacted. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17695: Don't warn user about solrUrl format when they do not supply it [solr]
epugh merged PR #3246: URL: https://github.com/apache/solr/pull/3246 -- 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] POC: Test picocli [solr]
epugh commented on code in PR #3247:
URL: https://github.com/apache/solr/pull/3247#discussion_r1987164110
##
solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java:
##
@@ -227,6 +250,20 @@ protected void runCloudTool(CloudSolrClient
cloudSolrClient, CommandLine cli) th
new JSONWriter(arr, 2).write(report);
echo(arr.toString());
}
+
+ @Override
+ public int callTool() throws Exception {
+if (zkHost == null) {
Review Comment:
i wonder if we should have a more generic abstraction that isn't tied to
"zkHost" for detecting if we are in solr cloud mode? Maybe an explicit
method, because I feel like we are slowly hiding zkHost more and more, and the
client may just be a solrClient configured with a solrUrl. <-- this isn't
specific to your picocli change I know!
--
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-17043: Remove SolrClient path pattern matching [solr]
jkmuriithi commented on code in PR #3238:
URL: https://github.com/apache/solr/pull/3238#discussion_r1987458954
##
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##
@@ -185,8 +188,31 @@ public void setQueryParams(Set queryParams) {
this.queryParams = queryParams;
}
- /** This method defines the type of this Solr request. */
- public abstract String getRequestType();
+ /**
+ * Defines the intended type of this Solr request.
+ *
+ * Subclasses should typically override this method instead of {@link
+ * SolrRequest#getRequestType}. Note that changing request type can
break/impact request routing
+ * within various clients (i.e. {@link CloudSolrClient}).
+ */
+ protected SolrRequestType getBaseRequestType() {
+return SolrRequestType.UNSPECIFIED;
+ }
+
+ /**
+ * Pattern matches on the underlying {@link SolrRequest} to identify ADMIN
requests and other
+ * special cases. If no special case is identified, {@link
SolrRequest#getBaseRequestType()} is
+ * returned.
+ */
+ public SolrRequestType getRequestType() {
+if (CommonParams.ADMIN_PATHS.contains(getPath())) {
+ return SolrRequestType.ADMIN;
+} else if (this instanceof IsUpdateRequest) {
Review Comment:
Honestly I wasn't a huge fan of this when I wrote it. My goal was just to
avoid disrupting existing code that relied on `IsUpdateRequest`, in order to
make this less of a breaking change.
--
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] Make test connectionLoss logic the same, add waitForLoss option [solr]
HoustonPutman merged PR #3225: URL: https://github.com/apache/solr/pull/3225 -- 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
[jira] [Commented] (SOLR-17510) Umbrella: CBOR, SolrJ compatible
[
https://issues.apache.org/jira/browse/SOLR-17510?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17934031#comment-17934031
]
David Smiley commented on SOLR-17510:
-
[~gus], responding to you months later but the direction we're going in is to
improve the maintenance of the Solr project by avoiding custom formats we need
to support – basically, longer term removing "javabin". CBOR is comparable and
yet we don't need to maintain it. In theory it should only be marginally more
work than JSON *if* Solr's JSON support was Jackson based, but it isn't. A
stepping stone on this journey is switching Jackson. This direction is linked
to the (new) V2 API because that API is POJO marshall/unmarshall based (with
annotations), using Jackson. Javabin doesn't have a role in this future.
{quote}We should be using JsonMapResponseParser (which is Jackson based)
{quote}
Correcting myself – it is {*}not{*}; it's Noggit based.
> Umbrella: CBOR, SolrJ compatible
>
>
> Key: SOLR-17510
> URL: https://issues.apache.org/jira/browse/SOLR-17510
> Project: Solr
> Issue Type: Improvement
> Components: SolrJ
>Reporter: David Smiley
>Priority: Major
>
> Here we propose a CBOR compatible SolrJ. Today, SolrJ only supports javabin
> and XML.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SolrJ ResponseParser API improvements, minor [solr]
epugh commented on code in PR #3248:
URL: https://github.com/apache/solr/pull/3248#discussion_r1987149392
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/NoOpResponseParser.java:
##
Review Comment:
Not specific to your changes, but a bit of JavaDocs on why you would use a
NoOpResponseParser might be nice.
##
solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java:
##
@@ -423,14 +423,8 @@ private String makeXml(String s) {
}
}
- private static final ResponseParser RAW_XML_RESPONSE_PARSER = new
NoOpResponseParser();
- private static final ResponseParser RAW_JSON_RESPONSE_PARSER =
- new NoOpResponseParser() {
-@Override
-public String getWriterType() {
- return "json";
-}
- };
+ private static final ResponseParser RAW_XML_RESPONSE_PARSER = new
NoOpResponseParser("xml");
Review Comment:
This is much nicer, versus what we had before.
##
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##
@@ -100,18 +100,15 @@ public String getWriterType() {
}
@Override
- public NamedList processResponse(InputStream body, String
encoding) {
-try {
- if (body.read() >= 0) readFile.set(true);
-} catch (IOException e) {
- throw new RuntimeException(e);
-}
+ public NamedList processResponse(InputStream body, String
encoding)
+ throws IOException {
+if (body.read() >= 0) readFile.set(true);
return null;
}
@Override
- public NamedList processResponse(Reader reader) {
-throw new UnsupportedOperationException("TODO unimplemented"); //
TODO
+ public Set getContentTypes() {
+return Set.of(); // don't enforce
Review Comment:
if you don't know about the previous `UOE` usage, then the comment doesn't
seem useful. I think it isn't adding value.
##
solr/core/src/test/org/apache/solr/response/TestRawTransformer.java:
##
@@ -283,12 +283,7 @@ public void testJsonTransformer() throws Exception {
strResponse.contains("\"links\":[\""));
}
- private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser();
+ private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser("xml");
private static final NoOpResponseParser JSON_NOOP_RESPONSE_PARSER =
- new NoOpResponseParser() {
-@Override
-public String getWriterType() {
- return "json";
-}
- };
+ new NoOpResponseParser("json");
Review Comment:
could this fit on line 287? regardless, this is much niver...
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##
@@ -622,7 +622,7 @@ protected NamedList executeMethod(
}
final Collection processorSupportedContentTypes =
processor.getContentTypes();
- if (processorSupportedContentTypes != null &&
!processorSupportedContentTypes.isEmpty()) {
+ if (!processorSupportedContentTypes.isEmpty()) {
Review Comment:
Loving this!
##
solr/solrj/src/java/org/apache/solr/client/solrj/ResponseParser.java:
##
@@ -16,45 +16,31 @@
*/
package org.apache.solr.client.solrj;
+import java.io.IOException;
import java.io.InputStream;
-import java.io.Reader;
import java.util.Collection;
-import java.util.Set;
import org.apache.solr.common.util.NamedList;
/**
+ * SolrJ Solr response parser.
+ *
* @since solr 1.3
*/
public abstract class ResponseParser {
- public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList processResponse(InputStream body, String
encoding);
+ /** The writer type placed onto the request as the {@code wt} param. */
+ public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList processResponse(Reader reader);
-
- /**
- * A well-behaved ResponseParser will return its content-type.
- *
- * @return the content-type this parser expects to parse
- * @deprecated use {@link #getContentTypes()} instead
- */
- @Deprecated
Review Comment:
nice to see deprecated code actually be removed!
##
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##
@@ -99,13 +99,9 @@ public String getWriterType() {
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
-public NamedList processResponse(InputStream body, String
encoding) {
- try {
-Map m = (Map) SmileWriterTest.decodeSmile(body);
-return new NamedList(m);
- } catch (IOException e) {
-throw new RuntimeException(e);
- }
+public NamedList processResponse(InputStream body, String
encoding) throws IOException {
Review Comment:
the wisdom of throwing a IOE shows up through out these changes
Re: [PR] SOLR-17685: Remove script creation of solr url based on SOLR_TOOL_HOST in favour of java code in CLI tools [solr]
epugh commented on code in PR #3223:
URL: https://github.com/apache/solr/pull/3223#discussion_r1987194246
##
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##
@@ -96,7 +98,7 @@ public class AuthTool extends ToolBase {
.argName("FILE")
.required()
.desc(
- "This is where any authentication related configuration files,
if any, would be placed.")
+ "This is where any authentication related configuration files,
if any, would be placed. Defaults to $SOLR_HOME.")
Review Comment:
I don't think using these outside the CLI scripts is actually a true
scenario... In fact, I think if folks are using these tools outside of the
bin/solr scripts (like some folks have their own start scripts) it points to
weaknesses in bin/solr that we need to improve!
--
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-17685: Remove script creation of solr url based on SOLR_TOOL_HOST in favour of java code in CLI tools [solr]
epugh commented on code in PR #3223:
URL: https://github.com/apache/solr/pull/3223#discussion_r1987200962
##
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##
@@ -75,8 +75,10 @@ public class AuthTool extends ToolBase {
.longOpt("solr-include-file")
.hasArg()
.argName("FILE")
+ .required()
Review Comment:
At least right now, we have logic that if this file does NOT exist or is not
writable, then the AuthTool quits with a warning. So I think making it
required is the right thing. Having said that, longer term maybe something to
think about? Becasue it makes it implicitly assumed that the bin/solr auth
command runs on your Solr server!
--
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] SolrJ ResponseParser API improvements, minor [solr]
dsmiley commented on code in PR #3248:
URL: https://github.com/apache/solr/pull/3248#discussion_r1987307957
##
solr/core/src/test/org/apache/solr/response/TestRawTransformer.java:
##
@@ -283,12 +283,7 @@ public void testJsonTransformer() throws Exception {
strResponse.contains("\"links\":[\""));
}
- private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser();
+ private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser("xml");
private static final NoOpResponseParser JSON_NOOP_RESPONSE_PARSER =
- new NoOpResponseParser() {
-@Override
-public String getWriterType() {
- return "json";
-}
- };
+ new NoOpResponseParser("json");
Review Comment:
no; all these words really add up and it barely line wraps
--
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-17607: Http ClusterStateProvider, lazy connect [solr]
mlbiscoc commented on code in PR #3249:
URL: https://github.com/apache/solr/pull/3249#discussion_r1987240404
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -230,43 +214,41 @@ private SimpleOrderedMap submitClusterStateRequest(
}
@Override
- public Set getLiveNodes() {
+ public synchronized Set getLiveNodes() {
+// synchronized because there's no value in multiple doing this at the
same time
if (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp),
TimeUnit.NANOSECONDS)
-> getCacheTimeout()) {
+<= getCacheTimeout()) {
+ return this.liveNodes; // cached copy is fresh enough
+}
+if (liveNodes != null) { // thus we've fetched liveNodes previously
Review Comment:
Nevermind ignore what I said. I misinterpreted this...
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -78,28 +78,12 @@ public void init(List solrUrls) throws Exception {
}
})
.collect(Collectors.toList());
+this.urlScheme = this.configuredNodes.get(0).getProtocol();
Review Comment:
Cool, yeah small change and I like this a lot better!
--
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
