[GitHub] [lucene-solr] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-526961393 Test is failing also before this commit, in both build environments ant and eclipse on my development host, so i ignored it locally. Does not seem to be related to this PR. All other tests are running successfully. 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] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-507243090
> I was able to clean up some of the failures locally by removing the second
clause in the if-statement below. (This was added in this PR in
EmbeddedSolrServer)
>
> ```
> private Set getContentStreams(final SolrRequest
request) throws IOException {
> if (request.getMethod() == SolrRequest.METHOD.GET || request
instanceof QueryRequest) return null;
> ```
>
> This makes sensethere are some QueryRequest implementations that are
supposed to be POST's. (e.g. JSON-queries and JSON-faceting requests, Solr
tagging requests, etc.). But there are still other failing tests that rely on
SolrJettyTestBase. So I haven't figured it out yet, but something's still not
quite right in that base-class.
>
> With Cao Manh Dat working on the 8.1.2 release, and Ignacio Vera talking
about an 8.2 shortly afterward, if we can't figure out the test issues soon, it
might be best to split the functionality changes and test-base changes into two
separate PR's if possible. I'd hate for this fix to miss the next crop of
releases because we couldn't cross the finish line on test-changes also bundled
into this commit.
Had some problems with the generated ivy cache, but after all i managed to
run all test successfully. The new PR is #755 , so have a look, if merged i can
update this one. The corresponding issue is SOLR-13592
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] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-507243090
> I was able to clean up some of the failures locally by removing the second
clause in the if-statement below. (This was added in this PR in
EmbeddedSolrServer)
>
> ```
> private Set getContentStreams(final SolrRequest
request) throws IOException {
> if (request.getMethod() == SolrRequest.METHOD.GET || request
instanceof QueryRequest) return null;
> ```
>
> This makes sensethere are some QueryRequest implementations that are
supposed to be POST's. (e.g. JSON-queries and JSON-faceting requests, Solr
tagging requests, etc.). But there are still other failing tests that rely on
SolrJettyTestBase. So I haven't figured it out yet, but something's still not
quite right in that base-class.
>
> With Cao Manh Dat working on the 8.1.2 release, and Ignacio Vera talking
about an 8.2 shortly afterward, if we can't figure out the test issues soon, it
might be best to split the functionality changes and test-base changes into two
separate PR's if possible. I'd hate for this fix to miss the next crop of
releases because we couldn't cross the finish line on test-changes also bundled
into this commit.
Had some problems with the generated ivy cache, but after all i managed to
run all test successfully. The new PR is #755 , so have a look, if merged i can
update this one. The corresponding issue is SOLR-13592.
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] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-507243090
> I was able to clean up some of the failures locally by removing the second
clause in the if-statement below. (This was added in this PR in
EmbeddedSolrServer)
>
> ```
> private Set getContentStreams(final SolrRequest
request) throws IOException {
> if (request.getMethod() == SolrRequest.METHOD.GET || request
instanceof QueryRequest) return null;
> ```
>
> This makes sensethere are some QueryRequest implementations that are
supposed to be POST's. (e.g. JSON-queries and JSON-faceting requests, Solr
tagging requests, etc.). But there are still other failing tests that rely on
SolrJettyTestBase. So I haven't figured it out yet, but something's still not
quite right in that base-class.
>
> With Cao Manh Dat working on the 8.1.2 release, and Ignacio Vera talking
about an 8.2 shortly afterward, if we can't figure out the test issues soon, it
might be best to split the functionality changes and test-base changes into two
separate PR's if possible. I'd hate for this fix to miss the next crop of
releases because we couldn't cross the finish line on test-changes also bundled
into this commit.
Had some problems with the generated ivy cache, but after all i managed to
run all test successfully. The new PR is #755 , so have a look, if merged i can
update this one
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] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-506257792 Already had time to review? All tests should pass now. 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] thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539
thomaswoeckinger edited a comment on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-504697943 > Hey Thomas, the code compiles and passes `precommit` now, which is a good step, but it looks like there are some test failures that this PR introduces. It looks like they're related to a change you made to SolrJettyTestBase. See one of my inline comments for more details. I also asked a few questions that I'd be interested in answers to before merging. > No problem, code can only be getting better, and there are always reasons when questions are asked. > Let me know if you're having any trouble reproducing those test failures locally. > No troubles but as i mentioned before, working with the generated eclipse project is really a pain! > Sorry that this has gone back and forth so many rounds. I imagine you're likely frustrated. But we're very close to getting this merged. Thanks for your patience and hard work on this. You don't have to be sorry about that, on my side we are using solr server pretty much, so i am always interested in better test coverage an cleaner code base. 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
