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 length method to RandomAccessInput #12594

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Sep 26, 2023

Implementation is straight forward as we always have the length of the data when constructing a RandomAccessInput. The only odd thing is that ByteBuffersDataInput already has a method called size() so I deprecated it in favour of this new one.

closes #12592

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.

LGTM. Add a CHANGES entry?

@iverase
Copy link
Contributor Author

iverase commented Sep 26, 2023

Thanks @jpountz! What it is not clear to me is if adding a method to this interface is considered a breaking change and it can only be introduced in a major release, or if it can be backported to the 9.x line. Any suggestions?

@romseygeek
Copy link
Contributor

I think it's fine to add simple methods to interfaces like this in point releases. Extending RandomAccessInput would be a pretty expert use of lucene.

One suggestion: given that ByteBuffersDataInput already has a size method, why not name the new interface method size as well? The two terms are more or less interchangeable, and that way you don't need to do all the deprecations.

@iverase
Copy link
Contributor Author

iverase commented Sep 27, 2023

One suggestion: given that ByteBuffersDataInput already has a size method, why not name the new interface method size as well? The two terms are more or less interchangeable, and that way you don't need to do all the deprecations.

Because IndexInput already defines a method called length, so adding a size method means that all the IndexInput that implements RandomAccessInput (which there are a few) will have a size and a length method which looks less ideal. But if folks thinks otherwise I don't feel strongly about it.

@romseygeek
Copy link
Contributor

Because IndexInput already defines a method called length, so adding a size method means that all the IndexInput that implements RandomAccessInput (which there are a few) will have a size and a length method which looks less ideal

Ah right, no your way makes more sense then.

@iverase iverase merged commit 6d764c3 into apache:main Sep 27, 2023
@iverase iverase deleted the randomaccessinput-length2 branch September 27, 2023 11:00
iverase added a commit that referenced this pull request Sep 27, 2023
 Add RandomAccessInput#length method to the RandomAccessInput interface. In addition it deprecates
  ByteBuffersDataInput#size in favour of this new method.
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.

Add length method to RandomAccessInput
3 participants