Fix segmentInfos replace doesn't set userData #12626
Conversation
Shibi-bala
force-pushed
the
sb/fixSegmentInfosReplace
branch
from
e970a91
to
68f0a22
Compare
|
Hi @Shibi-bala and great to see you here. Let's sync up this week and maybe we can help move this PR forward. It's a good catch, so thank you. |
| @@ -1996,6 +1996,41 @@ public void testGetCommitData() throws Exception { | |||
| dir.close(); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
It was the case that Lucene unit tests preferred to avoid the @Test annotation, going instead with the JUnit 3 convention where public methods whose name start with test... would indicate test methods.
I think the annotation on testGetCommitData() above must have slipped through (11 years ago).
There was a problem hiding this comment.
Looking through the other tests, I think this is an opportunity to clean up that @Test annotation from testGetCommitData() to make the whole test class consistent (since that's the only other occurrence of @Test here).
There was a problem hiding this comment.
Good callout, fixed
|
Kind of confused why this check is failing. This was never changed and I've tried merging. 1```
|
|
Have you merged in the latest main branch, so this PR is uptodate? This could be an issue which already existed when the PR was created. |
|
@uschindler Ah I needed to re-sync my forked repo 😅 |
|
Looks good now. |
|
@uschindler hey, thanks for the approval! Read the contributing guidelines, but not entirely sure how to get permissions to merge this PR. |
|
You can't do it. Please add a Changes entry unter the 9.9 section, commit it to branch and I will merge and Backport your PR. I am just away from my computer at moment, sorry for the delay. |
|
@Shibi-bala It's here: Line 147 in c228e4b You need to include your user and the GitHub user(s) that reviewed it/assisted as seen in the other entries. |
|
Made the changes. Thanks @uschindler @MarcusSorealheis @msfroh 😁 |
Description
The replace method of
SegmentInfosdoes not overwrite theuserDatafield. This PR fixes that bug. Relevant issue is opened here.This closes #12637