Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lucene new tests #1735

Closed
wants to merge 5 commits into from
Closed

Lucene new tests #1735

wants to merge 5 commits into from

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented Mar 25, 2013

Mostly @andyuk1986 's changes, I added some additional commits.

@@ -22,6 +22,7 @@
*/
package org.infinispan.lucene;

import junit.framework.Assert;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions should come from org.testng.AssertJUnit to avoid mixing up libraries... unless we've started porting over testsuites over to JUnit but I don't think that's the case yet?

@galderz
Copy link
Member

galderz commented Mar 27, 2013

Finally, tests fail in my environment: https://gist.github.com/galderz/5253057

@Sanne
Copy link
Member Author

Sanne commented Mar 27, 2013

Great points @galderz I missed those in my review. @andyuk1986 are you going to have time to address those comments or shall I look into it (tomorrow)?
I'd like to merge this soon so we can move on ;)

I also had it fail tests often in my case - at least before I added those isolation commits - so it makes me think that to improve he process next time it would be nice if you could provide a smaller pull rather than adding hundreds of tests in one go.
At least in my case I tend to look less at the tiny detail with such large changes.. so kudos to @galderz for still paying attention to the smaller but nasty details like unclosed resources.

@galderz
Copy link
Member

galderz commented Mar 27, 2013

@Sanne Depends on the code itself. You are more familiar with the inners of lucene...etc, so I assumed that you'd be looking at the tests in greater detail, whereas I limited myself to the smaller details, avoid leaks...etc :)

@Sanne
Copy link
Member Author

Sanne commented Mar 27, 2013

@galderz thanks. I should have noticed those anyway, I'm getting old :-P

there's not much low level Lucene in thewhole module, I'd expect you all to be able to look at the full implementation without needing my advice: tricky concurrency expectations from Lucene's usage are highlighted in comments. Overall, it's quite simple: we store byte[] in Infinispan rather than a filesystem, and use some locking tricks to provide e relatively sane read isolation to all nodes; if you ignore the various key types and externalizers and interfaces this code is made of three classes.

@andyuk1986
Copy link
Contributor

@Sanne how shall I proceed here? shall I fix Galder's remarks and push the changes to your repo branch?

@andyuk1986
Copy link
Contributor

@Sanne I'll make changes and will send a pull request to your repo

@Sanne
Copy link
Member Author

Sanne commented Mar 27, 2013

@andyuk1986 thanks looks good, or just send the pull to infinispan/master and close my pull.
Can you also investigate on Galder's test failure?

@andyuk1986
Copy link
Contributor

Hi all,

I've fixed all the remarks and also fixed the failing tests. You can find the changes in the new pull request: #1758

I think this pull request can be closed.

Regards,
Anna.

@Sanne Sanne closed this Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants