madrob commented on a change in pull request #585:
URL: https://github.com/apache/solr/pull/585#discussion_r821894872
##########
File path: solr/core/build.gradle
##########
@@ -90,6 +90,7 @@ dependencies {
implementation 'org.apache.httpcomponents:httpcore'
compileOnlyApi 'javax.servlet:javax.servlet-api'
+ compileOnlyApi 'org.eclipse.jetty.toolchain:jetty-servlet-api'
Review comment:
I think having both of these is incorrect? From a visual inspection it
looks like jetty-servlet-api has everything that java.servlet-api has, and
more. They definitely both end up in `server/lib` not sure if that's from here
or from somewhere else.
##########
File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java
##########
@@ -68,8 +69,12 @@ public static void processParams(SolrRequestHandler handler,
SolrQueryRequest re
String[] jsonFromParams = map.remove(JSON); // params from the query
string should come after (and hence override) JSON content streams
for (ContentStream cs : req.getContentStreams()) {
+ // if BinaryResponseParser.BINARY_CONTENT_TYPE, let the following fail
below - we may have adjusted the content without updating the content type
+ // problem in this case happens in a few tests, one seems to happen
with kerberos and remote node query (HttpSolrCall's request proxy)
Review comment:
It took a bit of digging, but I finally figured it out. When Jetty
receives a request with no body content, it creates a placeholder zero-length
application/octet-stream content object. Then when it gets a redirect (like an
explicit redirect handler, or some authentication plugins cause), it copies
that body to the new request. When the new request gets to our search handlers,
we see the body and save it as a stream. So I think that we could check for
that in `SolrRequestParsers.parseParamsAndFillStreams` - the combination of a
GET request with a zero length octet-stream body seems like a safe instance to
drop the stream. Maybe we could drop all the stream bodies for all get
requests, I'm not sure. This could be a bug in Jetty, but I haven't figured out
a simple repro case for them yet.
##########
File path:
solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
##########
@@ -320,17 +340,113 @@ protected URI getBaseUri() throws URISyntaxException {
}
private void pushObject(String path, String content) {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
s3.putObject(b -> b.bucket(BUCKET_NAME).key(path),
RequestBody.fromString(content));
}
}
private File pullObject(String path) throws IOException {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
File file = temporaryFolder.newFile();
InputStream input = s3.getObject(b -> b.bucket(BUCKET_NAME).key(path));
FileUtils.copyInputStreamToFile(input, file);
return file;
}
}
+
+ public S3Client createS3ClientV2() {
Review comment:
I don't think we need this - tried running tests locally without it and
using S3_MOCK_RULE to build the client directly and things seemed fine.
--
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
