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

Fix SegmentInfos replace doesn't update userData #948

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

tohidemyname
Copy link
Contributor

@tohidemyname tohidemyname commented Jun 5, 2024

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Fixes #{bug number} (in this specific format)
#947

Description

{Detail}

@paulirwin
Copy link
Contributor

@tohidemyname Thanks for this PR!

@NightOwl888 This appears to be a pretty straightforward port of apache/lucene#12626 which was in Lucene 9.9.0. Given the single-line nature of the change, I think this is probably safe to backport. Let me know if you agree. I'll tidy it up with a comment explaining the backport so we're aware for future porting efforts.

@NightOwl888
Copy link
Contributor

NightOwl888 commented Oct 22, 2024

@paulirwin - Thanks for finding that PR. The fact that a link or mention of it was missing from both the issue and PR was the primary thing holding this up.

I see a few issues that should be addressed before merging it.

  • 1. The production change is missing a LUCENENET comment indicating where the change (which diverges from 4.8.0) came from. Ideally, there would be a link back to the PR or commit where the code was ported from.
  • 2. The name of the test method should be Pascal case. NUnit doesn't automatically detect test methods based on a naming convention, so there is no need to stick with that casing in .NET.
  • 3. The test method and GetCommitData() method are not in the same order in the file as they were in Lucene. In Lucene, TestVariableSchema() (the following test in this PR) is on line 593 and TestGetCommitDataFromOldSnapshot() is on line 2064.
  • 4. The original test used getLiveCommitData(), not getCommitData(). It's fine if setLiveCommitData() doesn't exist in 4.8.0, but we should LUCENENET comment to make it clear why we diverged (and that it has diverged) on each of the lines that call GetCommitData() and SetCommitData(), since these will need to be changed back when we upgrade.
  • 5. The original test used newSnapshotIndexWriterConfig(null). We should port that method in LuceneTestCase rather than inlining the configuration, and add a LUCENENET comment indicating where the diverging method came from.
  • 6. The TestGetCommitDataFromOldSnapshot() and GetCommitData() methods also don't have a LUCENENET comment indicating where these diverging changes came from.

@paulirwin
Copy link
Contributor

@NightOwl888 My thoughts exactly, thanks for writing that all out. I'll make those changes to this PR.

@paulirwin paulirwin self-assigned this Oct 22, 2024
@paulirwin paulirwin added this to the 4.8.0-beta00018 milestone Oct 24, 2024
src/Lucene.Net.Tests/Index/TestIndexWriter.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriter.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriter.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriter.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriter.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Thanks @paulirwin and @tohidemyname, this looks good to merge.

@NightOwl888 NightOwl888 merged commit 0d25d8f into apache:master Nov 5, 2024
199 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants