Fixes SOLR-13539

[thomaswoeckinger]

Introduce EmbeddedSolrTestBase for easy and simple integration tests, fixes are included for SOLR-13331 and SOLR-13347.

@gerlowskija Please review

[thomaswoeckinger]

Found two other bugs when using different codec than JavaBinCodec, will do another PR
@gerlowskija Did you have time to review the PR?

[thomaswoeckinger]

@ErickErickson How to get reviewer for this PR Jason seems to be busy

[ErickErickson]

Unfortunately this is an ongoing issue. Usually, people will gently nudge with additional comments on the JIRA. I’m slammed for time too unfortunately.

…

On May 10, 2019, at 1:49 AM, Thomas Wöckinger ***@***.***> wrote: @ErickErickson How to get reviewer for this PR Jason seems to be busy — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dsmiley]

It'd help to review your changes if you made fewer arbitrary changes, like adding 'final' and changing indentation of javadocs that were fine as they were.
Also, it'd help to summarize why 3 different issues are being fixed in one PR. Might be just fine but please add info/context to make reviewer's job either or you may not get a review at all.

[ErickErickson]

There are two things that might help with the arbitrary change issue: 1> On the reviewing side, In InteliJ, I can choose an option “ignore all whitespace”, I’m sure other tools have similar. Doesn’t help with final and the like of course. 2> Again in IntelliJ and (presumably) other tools I can set the autoformat to only reformat changed lines rather than entire files. FWIW, Erick

…

On May 15, 2019, at 6:49 AM, David Smiley ***@***.***> wrote: It'd help to review your changes if you made fewer arbitrary changes, like adding 'final' and changing indentation of javadocs that were fine as they were. Also, it'd help to summarize why 3 different issues are being fixed in one PR. Might be just fine but please add info/context to make reviewer's job either or you may not get a review at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[thomaswoeckinger]

The files where formatted with settings generated by 'ant eclipse', so they where wrong before, i can remove the final modifier which i personally prefer. Some additional inflammations about the issues: The issues are related to each other due to the change from JavaBinCodec to use UTF8CharSequence instead of String, which leads to a collection.remove calls which are useless because their type does not match. The issue was introduced in version 7.7.x and initially detected on our side by integration tests. I want to make sure that this issues will not occur again in the future so i added missing unit tests and refactored the test base to allow easy exchange of the used codec. The test written for SOLR-13331 shows up two additional issues described by SOLR-13347 and SOLR-11841. Also some unexpected internal state where caused by the EmbeddedSolrServer which i corrected to (return null stream instead of empty). So i will remove the final modifiers and push again! Thx for your time! Best, Tom Erick Erickson <notifications@github.com> schrieb am Mi., 15. Mai 2019, 19:56:

…

There are two things that might help with the arbitrary change issue: 1> On the reviewing side, In InteliJ, I can choose an option “ignore all whitespace”, I’m sure other tools have similar. Doesn’t help with final and the like of course. 2> Again in IntelliJ and (presumably) other tools I can set the autoformat to only reformat changed lines rather than entire files. FWIW, Erick > On May 15, 2019, at 6:49 AM, David Smiley ***@***.***> wrote: > > It'd help to review your changes if you made fewer arbitrary changes, like adding 'final' and changing indentation of javadocs that were fine as they were. > Also, it'd help to summarize why 3 different issues are being fixed in one PR. Might be just fine but please add info/context to make reviewer's job either or you may not get a review at all. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#665?email_source=notifications&email_token=ACKCGQE2FNGKHHJ6EP6WVA3PVRFFBA5CNFSM4HLKBL4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVPOJZQ#issuecomment-492758246>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACKCGQCSSE62WLOMMUEHKLLPVRFFBANCNFSM4HLKBL4A> .

[janhoy]

Yes, please do only feature related changes in the PR, then you can file another JIRA for other formatting changes if needed. I too keep fighting with the IDE that wants to auto-format things. But it only creates unnecessary noise for reviewers :-(

[thomaswoeckinger]

Pushed again, hope it is enough. Waiting for your suggestions...

[janhoy]

There is stil a TON of unrelated whitespace changes in the patch. If you want to reformat code, best practice is to open another PR for that separately.

What I normally do to clean up is first disable the aggressive reformatting feature in the IDE, then open "Compare with branch ..." in my IDE (IntelliJ) and in the diff view, click restore on every single diff change that is messed up by the IDE, then commit and push those changes. You'll then see that your diff becomes MUCH smaller and easier to read!

[martin-g]

One can easily hide the white space changes in Github UI by pressing the No whitespace button (or add manually ?w=1 to the url).

[thomaswoeckinger]

Is it better for you if put the formatting in an seperate commit, i think if i create a new pr this will never get merged, otherwise the format would not be in the current state. Jan Høydahl <notifications@github.com> schrieb am Do., 16. Mai 2019, 15:19:

…

There is stil a TON of unrelated whitespace changes in the patch. If you want to reformat code, best practice is to open another PR for that separately. What I normally do to clean up is first disable the aggressive reformatting feature in the IDE, then open "Compare with branch ..." in my IDE (IntelliJ) and in the diff view, click restore on every single diff change that is messed up by the IDE, then commit and push those changes. You'll then see that your diff becomes MUCH smaller and easier to read! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#665?email_source=notifications&email_token=ACKCGQEYP7AEC5CH64ZIUB3PVVNPJA5CNFSM4HLKBL4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVRY7YI#issuecomment-493064161>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACKCGQAVP6KGSZBUSHKBPRLPVVNPJANCNFSM4HLKBL4A> .

[thomaswoeckinger]

I splitt up the changes into an additional commit, i think review is pretty easy now if you select next to last in the commit view.

Best,

Tom

[janhoy]

It looks trivial to do a separate PR for SOLR-13347. Please do that if possible and let's avoid huge multi-issue patches.

I am still confused by unrelated formatting changes, consider opening a new JIRA and PR for those, I'm sure someone will take a look at it :)

You might want to review the contributor guide: https://wiki.apache.org/solr/HowToContribute#Creating_the_patch_file

[thomaswoeckinger]

Added PR for UUID only, will change this PR when it is merged, because unit test will fail otherwise

[janhoy]

Thanks. I'm not saying I have the capacity or skills to commit these, but at least trying to help you shape your contributions into bite-sized parts that more likely attract committer attention. Once you remove all white-space edits and SOLR-1347 code from this PR it will be a lot easier to chew over.

[noblepaul]

Overall the patch looks good. But I'm still not sure I have looked at everytging because some of these are formatting changes and adding final to variables

[thomaswoeckinger]

Will update this PR when #681 is merged, so @noblepaul and @janhoy plz have a look

[thomaswoeckinger]

As i already mentioned on #755, any plans on merging this back to 8.x?

[thomaswoeckinger]

One test is failing, testUserAndTestDefaultConfigsetsAreSame from TestConfigSetsAPI, have to investigate if it fails before or not

[thomaswoeckinger]

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.

[gerlowskija]

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.

[thomaswoeckinger]

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.

[thomaswoeckinger]

Removed JavaDoc reference to EmbeddedSolrServer as suggested by @dsmiley

[thomaswoeckinger]

@gerlowskija ready to merge?

[thomaswoeckinger]

Rebased on the PR #883

[thomaswoeckinger]

@gerlowskija: would be great if we can get this into 8.3, which will start in a about 2 weeks

[gerlowskija]

Hi Thomas. I'd really like to get this (#665) in for 8.3, but right now it's bundled to your change in #883. I can't merge this without #883.

And I haven't had the time I need to understand how the different pieces of #883 (response formats, field types, binary content) fit together and what the best design is there.

So what I'm going to do is try to unbundle the two PRs, or at least flip the ordering. I'll take what you've uploaded to #665 here and either comment out or remove entirely the tests that hit this binary-XML issue. They can be added back in #883, once binary-xml works for these field types.

There might still be time to get #883 in, as I have a chunk of time now that I didn't before. But even if we don't get to it for 8.3, it won't prevent #665 from getting in.

[thomaswoeckinger]

Hi Thomas. I'd really like to get this (#665) in for 8.3, but right now it's bundled to your change in #883. I can't merge this without #883.

And I haven't had the time I need to understand how the different pieces of #883 (response formats, field types, binary content) fit together and what the best design is there.

So what I'm going to do is try to unbundle the two PRs, or at least flip the ordering. I'll take what you've uploaded to #665 here and either comment out or remove entirely the tests that hit this binary-XML issue. They can be added back in #883, once binary-xml works for these field types.

There might still be time to get #883 in, as I have a chunk of time now that I didn't before. But even if we don't get to it for 8.3, it won't prevent #665 from getting in.

No problem i can comment out the binary test, rebase and push again.
Just a second!

[thomaswoeckinger]

Pushed again without ignored binary test

[thomaswoeckinger]

pre commit check is still toggling, it is working locally

[gerlowskija]

pre commit check is still toggling, it is working locally

Precommit has some known issues on master that make it a little flaky everywhere. But it does seem like it has a higher rate of failure on Github (on all PR's...it's not specific to this one). I wonder what that's about...

Running tests locally now. Will merge later this morning assuming things check out.

[thomaswoeckinger]

pre commit check is still toggling, it is working locally

Precommit has some known issues on master that make it a little flaky everywhere. But it does seem like it has a higher rate of failure on Github (on all PR's...it's not specific to this one). I wonder what that's about...

Running tests locally now. Will merge later this morning assuming things check out.

Great to hear.

[thomaswoeckinger]

@gerlowskija Any plans on getting this into 8.3?

[thomaswoeckinger]

Thx Jason for moving it to 8.3