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

Add the necessary steps for lucene replace #551

Merged
merged 4 commits into from
Oct 16, 2017
Merged

Add the necessary steps for lucene replace #551

merged 4 commits into from
Oct 16, 2017

Conversation

garyluoex
Copy link
Collaborator

No description provided.

@garyluoex
Copy link
Collaborator Author

One thing I'm not sure is the lock, two write locks together, not sure if it will work

@michael-mclawhorn
Copy link
Contributor

We need more tests for this, but that was true before. This fix seems right to me.

@@ -191,6 +192,7 @@ class LuceneSearchProviderSpec extends SearchProviderSpec<LuceneSearchProvider>
!Files.exists(file4)
}

@Ignore("This test is currently not valid because the replacement index is invalid.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't seem to be ignored, 'cause it was working previously

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It no longer works, because the file we are using is fake, the method now requires a real index file

@QubitPi
Copy link
Contributor

QubitPi commented Oct 13, 2017

Given this needs to be addressed in a separate PR, #552, I'm OK with it

QubitPi
QubitPi previously approved these changes Oct 13, 2017
@michael-mclawhorn michael-mclawhorn dismissed stale reviews from QubitPi and themself via cc2c619 October 13, 2017 21:44
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

I wish I had a test plan to propose. @QubitPi is planning to look into it.

QubitPi
QubitPi previously approved these changes Oct 13, 2017
@garyluoex garyluoex dismissed stale reviews from QubitPi and michael-mclawhorn via f2ffbca October 13, 2017 23:33
@garyluoex garyluoex force-pushed the FixReplace branch 2 times, most recently from ad97746 to 503cdca Compare October 15, 2017 18:39
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

I think this will address a missing piece on index replacement. Downstream consumers will have to test it until the included test can be rewritten using more realistic changes.

@QubitPi
Copy link
Contributor

QubitPi commented Oct 16, 2017

Thanks @michael-mclawhorn. Test will be my next urgent thing to do #552

@garyluoex garyluoex merged commit 2ee4259 into master Oct 16, 2017
@garyluoex garyluoex deleted the FixReplace branch October 16, 2017 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants