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

Convert IOContext, MergeInfo, and FlushInfo to record classes #13205

Merged
merged 14 commits into from
Mar 26, 2024

Conversation

uschindler
Copy link
Contributor

Followup to #13204: This converts IOContext to a record class. This has several positive effects:

  • equals, hashCode and toString are autogenerated by an invokedynamic, so we can't forget to handle it correctly
  • it is also immutable
  • all parameter checking logic can be moved to some special constructor without any parameters (this is new syntax since Java 17)

The IOContext has some crazy constructors, I left them in, but all have to delegate to the default constructor now.

@uschindler uschindler added this to the 10.0.0 milestone Mar 23, 2024
@uschindler uschindler requested a review from ChrisHegarty March 23, 2024 11:44
@uschindler uschindler self-assigned this Mar 23, 2024
@uschindler
Copy link
Contributor Author

I needed to change some consumers to call the getter methods instead of accessing fields (which are now hidden).

@uschindler
Copy link
Contributor Author

We should possibly convert other classes to records, too (like FlushInfo or MergeInfo if they are immutable).

@uschindler
Copy link
Contributor Author

I had to fix the missing-doclet javadoc parser to understand records. The easy fix was to treat them as "classes".

@uschindler uschindler changed the title Convert IOContext to record class Convert IOContext, MergeInfo, and FlushInfo to record classes Mar 23, 2024
@uschindler
Copy link
Contributor Author

I also converted the MergeInfo and FlushInfo classes to records. Very clean now.

Copy link
Member

@rmuir rmuir left a comment

Choose a reason for hiding this comment

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

thanks for fixing the missing-doclet to deal with records!

@uschindler
Copy link
Contributor Author

thanks for fixing the missing-doclet to deal with records!

This was a requirement, because building Javadocs failed without that fix. It is really nice that the code throws an exception when it hits an unknown element!

@uschindler
Copy link
Contributor Author

I will apply those changes after #13196, because if doing this before, backporting of the other PR gets harder (#13196 has changes to IOContext, too).

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Much nicer!

Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	lucene/core/src/java/org/apache/lucene/store/IOContext.java
@uschindler uschindler requested a review from ChrisHegarty March 25, 2024 20:04
@uschindler
Copy link
Contributor Author

uschindler commented Mar 25, 2024

Hi @ChrisHegarty,
can you have another look on the crazy randomAccess flag afetr I merged main into this. Especially the checks in the record's constructor should be checked again. I have no idea what flags are mutually exclusive. Maybe there are more combinations missing.

Luckily @jpountz opened #13211, can't wait to see this be resolved.

@ChrisHegarty
Copy link
Contributor

Hi @ChrisHegarty,
can you have another look on the crazy randomAccess flag afetr I merged main into this. Especially the checks in the record's constructor should be checked again. I have no idea what flags are mutually exclusive. Maybe there are more combinations missing.

The record constructor checks, while not pretty, seem right to me. This is a nice cleanup and somewhat easier to reason about.

@uschindler uschindler merged commit 8c4ec1d into apache:main Mar 26, 2024
3 checks passed
@uschindler uschindler deleted the dev/iocontext_to_record branch March 26, 2024 11:27
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.

4 participants