[GitHub] [lucene-solr] thomaswoeckinger commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-529799183 @gerlowskija ready to merge? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-527779102 Removed JavaDoc reference to EmbeddedSolrServer as suggested by @dsmiley 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-527455888 > > As i already mentioned on #755, any plans on merging this back to 8.x? > > Yep. Generally committers are encouraged to let things "cook" on master awhile before merging things back to release branches like `branch_8x`. This gives us more confidence that there's no weird flaky failures we've introduced. It's also more convenient to wait to merge commits back to other branches once all related commits have been put on master. Seems to be a good idea. > > I'm waiting mostly on the latter, since I've run the tests a good bit. So once this gets merged (and I merge the patch that Tim Owen put on SOLR-13539), then I'll move everything back to `branch_8x` all at once. Great > > As i already mentioned on #755, any plans on merging this back to 8.x? > > Yep. Generally committers are encouraged to let things "cook" on master awhile before merging things back to release branches like `branch_8x`. This gives us more confidence that there's no weird flaky failures we've introduced. It's also more convenient to wait to merge commits back to other branches once all related commits have been put on master. > > I'm waiting mostly on the latter, since I've run the tests a good bit. So once this gets merged (and I merge the patch that Tim Owen put on SOLR-13539), then I'll move everything back to `branch_8x` all at once. 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented 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, 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-526877311 One test is failing, testUserAndTestDefaultConfigsetsAreSame from TestConfigSetsAPI, have to investigate if it fails before or not 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-526873168 As i already mentioned on #755, any plans on merging this back to 8.x? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-518040563 So please, i put a lot of work into this, it would be really great if somebody could help to review and get this into the main line at least! 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-516759833 Last interaction is now one month ago, so can anyone takeover the review please? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-515045605
> 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.
Reverted the if statement, reason was that internal code requires the whole
content response to be null. Added DirectJsonQueryRequestFacetingEmbeddedTest
to prove functionality.
A general comment, it would be great to switch to junit 5, test cases could
be extracted to default interfaces and all test classes simply have to
implement it and provide their solr client.
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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-513734056 Rebased again, @gerlowskija it is two weeks ago since last notice, maybe someone else could takeover the review if jason is to busy? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-512198034 @gerlowskija Are you still working on this? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-511391449 Still 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. 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-509148931 Anything new? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-507547948 Rebased against PR #755, resolved conflicts, tests are all running, ready to merge after #755 is merged 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-507443102 > Ok. I was worried we'd miss 8.1.2, so I took your changes to AbstractEnumField, BinaryField, BoolField, and UUIDField and merged them in a much smaller commit of their own. (see here: [8242e6c](https://github.com/apache/lucene-solr/commit/8242e6ce1da141e5585c0a8cd7ffc5953c5bf035)) This commit didn't include any of your test changes, unfortunately. > > (I don't like to merge code without tests, but since I'd seen and run the tests you added in this PR specifically for those field types, and tested it manually, I felt pretty confident.) > > Anyway, I'll take a look at 755 and run the tests later today. Since your latest test changes are over in 755, and the atomic-update related field changes have already been merged, is there a reason to keep this PR around? We could just work off of 755 and close this out, unless there's a reason you see to keep both 755 and 665 open? The only reason is that the related issues are depending on each other, so i think it is still more clear to do this whole stuff with the PR's #755 and #665. Rebasing should be no problem anyway. 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented 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 #720, 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-506257792 Already had time to review? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented 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
[GitHub] [lucene-solr] thomaswoeckinger commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-502588814 Any comments on this? 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-501590607 Rebased and ready to review 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 commented on issue #665: Fixes SOLR-13539
thomaswoeckinger commented on issue #665: Fixes SOLR-13539 URL: https://github.com/apache/lucene-solr/pull/665#issuecomment-501586703 > PR looks good to me. Sorry for my tardiness in reviewing it. > > I have some qualms about introducing a new test base class entirely as this PR does. Our test-base situation is already quite jumbled and there's a lot of developer confusion around when each test-case should be used. There's also been some recent movement towards randomizing the SolrJ RequestWriter in one of the existing test bases. So it's hard to say whether adding a new test-base-class will hold up long term. But I don't want to let the perfect get in the way of the good here - especially since this PR adds some awesome test coverage for atomic updates of a large variety of field types. > > If Noble gets a chance to review #681 I'll circle back and merge this. (Assuming he doesn't beat me to it). You are right about the number of existing test bases, but from my point of view SolrJettyTestBase is hiding the usage of EmbeddedSolrServer, which no one is expecting to be used there. So i think it is clearer now. To get rid of this situation the whole test base have to be re-written, which is to much for this PR ;-) 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
