[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r320246350
##
File path: solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java
##
@@ -429,7 +434,18 @@ public SolrInputDocument readDoc(XMLStreamReader parser)
throws XMLStreamExcepti
break;
} else if ("field".equals(parser.getLocalName())) {
// should I warn in some text has been found too
-Object v = isNull ? null : text.toString();
+Object v;
Review comment:
So, just to make sure I understand: what is "binary XML" here? Is this XML
where some node in the XML doc has binary content? Is this normal XML where
the whole XML doc/string has been encoded using some binary format for
compression or quicker transmission?
> this was the reason binary XML support was not working at least since 6.6.2
Interesting. Is this something that Solr claimed to support or had support
for at some point? Or this is a new ability that Solr has never had that
you're adding here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r320246350
##
File path: solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java
##
@@ -429,7 +434,18 @@ public SolrInputDocument readDoc(XMLStreamReader parser)
throws XMLStreamExcepti
break;
} else if ("field".equals(parser.getLocalName())) {
// should I warn in some text has been found too
-Object v = isNull ? null : text.toString();
+Object v;
Review comment:
So, just to make sure I understand: what is "binary XML" here? Is this XML
where some node in the XML doc has binary content? Is this normal XML where
the whole XML doc/string has been encoded using some binary format for
compression or quicker transmission?
> this was the reason binary XML support was not working at least since 6.6.2
Interesting. Is this something that Solr claimed to support or had support
for at some point? Or this is a new ability that Solr has never had that
you're adding here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r319118987
##
File path: solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java
##
@@ -429,7 +434,18 @@ public SolrInputDocument readDoc(XMLStreamReader parser)
throws XMLStreamExcepti
break;
} else if ("field".equals(parser.getLocalName())) {
// should I warn in some text has been found too
-Object v = isNull ? null : text.toString();
+Object v;
Review comment:
Hey @thomaswoeckinger what's the purpose of this change here? I gather it's
allowing the XMLLoader to handle a binary base64 content-type, but was that
required to get a particular test passing? Is it something needed by
EmbeddedSolrServer? Something else?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r319118987
##
File path: solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java
##
@@ -429,7 +434,18 @@ public SolrInputDocument readDoc(XMLStreamReader parser)
throws XMLStreamExcepti
break;
} else if ("field".equals(parser.getLocalName())) {
// should I warn in some text has been found too
-Object v = isNull ? null : text.toString();
+Object v;
Review comment:
Hey @thomaswoeckinger what's the purpose of this change here? I gather it's
allowing the XMLLoader to handle a binary base64 content-type, but was that
required for a particular test change? Is it something needed by
EmbeddedSolrServer? Something else?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296343329
##
File path: solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java
##
@@ -140,29 +139,17 @@ public synchronized SolrClient getSolrClient() {
}
/**
- * Create a new solr client.
- * If createJetty was called, an http implementation will be created,
- * otherwise an embedded implementation will be created.
- * Subclasses should override for other options.
+ * Create a new solr client. If createJetty was called, an http
implementation will be created, otherwise an embedded
+ * implementation will be created. Subclasses should override for other
options.
*/
public SolrClient createNewSolrClient() {
-if (jetty != null) {
- try {
-// setup the client...
-String url = jetty.getBaseUrl().toString() + "/" + "collection1";
-HttpSolrClient client = getHttpSolrClient(url,
DEFAULT_CONNECTION_TIMEOUT);
-return client;
- }
- catch( Exception ex ) {
-throw new RuntimeException( ex );
- }
-} else {
- return new EmbeddedSolrServer( h.getCoreContainer(), "collection1" ) {
-@Override
-public void close() {
- // do not close core container
-}
- };
+try {
+ // setup the client...
+ final String url = jetty.getBaseUrl().toString() + "/" + "collection1";
+ final HttpSolrClient client = getHttpSolrClient(url,
DEFAULT_CONNECTION_TIMEOUT);
+ return client;
+} catch (final Exception ex) {
+ throw new RuntimeException(ex);
Review comment:
[-1] Either I missed this on an earlier review, or this change is new in the
latest iteration of this PR. (Publishing changes with git's "force-push"
option as you've been doing makes it hard to see which changes are new and
which have been around for a few iterations).
Anyway, I didn't notice this change previously, but some test failures drew
my attention to it.
When I check out your PR and merge master into it locally, I get several
test failures:
```
[junit4] Tests with failures [seed: D067023E71C9E8C8]:
[junit4] - org.apache.solr.update.RootFieldTest.testUpdateWithChildDocs
[junit4] -
org.apache.solr.update.RootFieldTest.testLegacyBlockProcessing
[junit4] -
org.apache.solr.handler.tagger.EmbeddedSolrNoSerializeTest.testAssertTagStreamingWithSolrTaggerRequest
[junit4] -
org.apache.solr.handler.tagger.EmbeddedSolrNoSerializeTest.testTag
[junit4] -
org.apache.solr.update.processor.AbstractAtomicUpdatesMultivalueTest.initializationError
```
I looked into the RootFieldTest failure, and it is failing because of an NPE
in this method:
```
[junit4] ERROR 0.01s | RootFieldTest.testLegacyBlockProcessing <<<
[junit4]> Throwable #1: java.lang.RuntimeException:
java.lang.NullPointerException
[junit4]> at
__randomizedtesting.SeedInfo.seed([84EEE01E1A0D0DB8:E5D4F6A2DBEAF389]:0)
[junit4]> at
org.apache.solr.SolrJettyTestBase.createNewSolrClient(SolrJettyTestBase.java:152)
[junit4]> at
org.apache.solr.SolrJettyTestBase.getSolrClient(SolrJettyTestBase.java:136)
[junit4]> at
org.apache.solr.update.RootFieldTest.testLegacyBlockProcessing(RootFieldTest.java:58)
[junit4]> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[junit4]> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[junit4]> at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[junit4]> at
java.base/java.lang.reflect.Method.invoke(Method.java:566)
[junit4]> at java.base/java.lang.Thread.run(Thread.java:834)
[junit4]> Caused by: java.lang.NullPointerException
[junit4]> at
org.apache.solr.SolrJettyTestBase.createNewSolrClient(SolrJettyTestBase.java:148)
[junit4]> ... 41 more
```
Was there a reason you removed the `jetty != null` check from this block,
and the corresponding `else` block that created an EmbeddedSolrServer if jetty
hadn't been called? It seems like those are still needed here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296343689
##
File path: solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java
##
@@ -178,13 +165,6 @@ public static void cleanUpJettyHome(File solrHome) throws
Exception {
}
}
- public static void initCore() throws Exception {
Review comment:
[Q] Why did you remove this? Was it causing you an issue somewhere?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296338706
##
File path:
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSpellCheckResponse.java
##
@@ -27,21 +29,22 @@
import org.junit.BeforeClass;
import org.junit.Test;
-import java.util.List;
+import junit.framework.Assert;
/**
* Test for SpellCheckComponent's response in Solrj
*
*
* @since solr 1.3
*/
-public class TestSpellCheckResponse extends SolrJettyTestBase {
+public class TestSpellCheckResponse extends EmbeddedSolrServerTestBase {
Review comment:
[Q] Why are you changing the base class of this test here? I'm fine with
introducing a new base test-class in this PR, and maybe we want to eventually
change other classes to use this new base. But I'd rather that happen in a
separate PR.
(That way, if tests start failing it's clear what's at fault. With the
changes bundled together like this it's harder to tell whether the bugfixes
caused any hypothetical test failures, or the test-changes themselves are
unsound.)
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296338967
##
File path:
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java
##
@@ -32,14 +33,15 @@
* Test for SuggesterComponent's response in Solrj
*
*/
-public class TestSuggesterResponse extends SolrJettyTestBase {
+public class TestSuggesterResponse extends EmbeddedSolrServerTestBase {
Review comment:
[Q] Ditto re: changing base class to EmbeddedSolrServerTestBase
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296338559
##
File path: solr/solrj/src/test/org/apache/solr/client/solrj/GetByIdTest.java
##
@@ -27,13 +27,13 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class GetByIdTest extends SolrJettyTestBase {
-
+public class GetByIdTest extends EmbeddedSolrServerTestBase {
Review comment:
[Q] Why are you changing the base class of this test here? I'm fine with
introducing a new base test-class in this PR, and maybe we want to eventually
change other classes to use this new base. But I'd rather that happen in a
separate PR.
(That way, if tests start failing it's clear what's at fault. With the
changes bundled together like this it's harder to tell whether the bugfixes
caused any hypothetical test failures, or the test-changes themselves are
unsound.)
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r296337538
##
File path:
solr/solrj/src/test/org/apache/solr/client/solrj/response/TermsResponseTest.java
##
@@ -15,31 +15,33 @@
* limitations under the License.
*/
package org.apache.solr.client.solrj.response;
+
import java.util.List;
-import junit.framework.Assert;
-import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.EmbeddedSolrServerTestBase;
import org.apache.solr.client.solrj.SolrQuery;
-import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.client.solrj.request.QueryRequest;
import org.apache.solr.client.solrj.response.TermsResponse.Term;
+import org.apache.solr.common.SolrInputDocument;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
+import junit.framework.Assert;
+
/**
* Test for TermComponent's response in Solrj
*/
-public class TermsResponseTest extends SolrJettyTestBase {
-
+public class TermsResponseTest extends EmbeddedSolrServerTestBase {
Review comment:
[Q] Why are you changing the base class of this test here? I'm fine with
introducing a new base test-class in this PR, and maybe we want to eventually
change other classes to use this new base. But I'd rather that happen in a
separate PR.
(That way, if tests start failing it's clear what's at fault. With the
changes bundled together like this it's harder to tell whether the bugfixes
caused any hypothetical test failures, or the test-changes themselves are
unsound.)
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
gerlowskija commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r295847529
##
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
##
@@ -104,35 +103,51 @@ public static void writeXML( SolrInputDocument doc,
Writer writer ) throws IOExc
writer.write("");
}
- private static void writeVal(Writer writer, String name, Object v, String
update) throws IOException {
+ private static void writeVal(Writer writer, String name, Object v, String
update)
+ throws IOException {
+boolean binary = false;
if (v instanceof Date) {
- v = ((Date)v).toInstant().toString();
+ v = ((Date) v).toInstant().toString();
} else if (v instanceof byte[]) {
byte[] bytes = (byte[]) v;
v = Base64.byteArrayToBase64(bytes, 0, bytes.length);
+ binary = true;
} else if (v instanceof ByteBuffer) {
ByteBuffer bytes = (ByteBuffer) v;
- v = Base64.byteArrayToBase64(bytes.array(),
bytes.position(),bytes.limit() - bytes.position());
+ v = Base64.byteArrayToBase64(bytes.array(), bytes.position(),
bytes.limit() - bytes.position());
+ binary = true;
}
XML.Writable valWriter = null;
-if(v instanceof SolrInputDocument) {
+if (v instanceof SolrInputDocument) {
final SolrInputDocument solrDoc = (SolrInputDocument) v;
valWriter = (writer1) -> writeXML(solrDoc, writer1);
-} else if(v != null) {
- final Object val = v;
- valWriter = (writer1) -> XML.escapeCharData(val.toString(), writer1);
+} else if (v != null) {
+ Object val = v;
+ if (binary) {
+valWriter = (writer1) -> writer1.write((String) val);
+ } else {
+valWriter = (writer1) -> XML.escapeCharData(val.toString(), writer1);
+ }
}
if (update == null) {
if (v != null) {
-XML.writeXML(writer, "field", valWriter, "name", name);
+if (binary) {
+ XML.writeXML(writer, "field", valWriter, "name", name, "dt",
XMLLoader.BINARY_BASE64);
Review comment:
[-1] When I pull this code down locally, I get compilation errors.
XMLLoader lives in solr-core. SolrJ doesn't depend on solr-core (it's the
other way around), so when I try to compile this PR, I get:
```
[javac] Compiling 716 source files to
/Users/jasongerlowski/checkouts/lucene-solr/solr/build/solr-solrj/classes/java
[javac]
/Users/jasongerlowski/checkouts/lucene-solr/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java:36:
error: package org.apache.solr.handler.loader does not exist
[javac] import org.apache.solr.handler.loader.XMLLoader;
```
We should move this constant to somewhere in SolrJ so that it can be seen in
both solr-core and solr-solrj.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
