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

Declare core.iterators package public API #1390

Closed
milleruntime opened this issue Oct 22, 2019 · 7 comments
Closed

Declare core.iterators package public API #1390

milleruntime opened this issue Oct 22, 2019 · 7 comments
Assignees
Milestone

Comments

@milleruntime
Copy link
Contributor

This is something that is long overdue but has been put off for good reasons. Some refactoring was done in 2.0 for classes in this package to not use internal types. SortedKeyValueIterator should be declared public API and I think this package is close to this goal where it could be done for 2.1

@milleruntime
Copy link
Contributor Author

Classes under core.iterators with impl imports:

LongCombiner.java:import org.apache.accumulo.core.clientImpl.lexicoder.AbstractLexicoder;
system/MapFileIterator.java:import org.apache.accumulo.core.sample.impl.SamplerConfigurationImpl;
system/SequenceFileIterator.java:import org.apache.accumulo.core.sample.impl.SamplerConfigurationImpl;
system/MultiIterator.java:import org.apache.accumulo.core.dataImpl.KeyExtent;
user/TimestampFilter.java:import java.text.SimpleDateFormat;
user/BigDecimalCombiner.java:import org.apache.accumulo.core.clientImpl.lexicoder.AbstractLexicoder;
user/SummingArrayCombiner.java:import org.apache.accumulo.core.clientImpl.lexicoder.AbstractLexicoder;

@ctubbsii
Copy link
Member

It's okay to have impl imports. It's just not okay for them to expose those types in their public/protected members. apilyzer-maven-plugin should catch violations of this.

@ctubbsii
Copy link
Member

We should probably move system iterators out of the way, because those should not be public API, but otherwise, I think this is fine.

@milleruntime
Copy link
Contributor Author

milleruntime commented Nov 13, 2019

Based on some discussion in #1415, iterators may need to be treated as SPI instead.

@milleruntime milleruntime reopened this Nov 13, 2019
@milleruntime
Copy link
Contributor Author

This is dependent on #1426

@keith-turner
Copy link
Contributor

I made a few changes to #1426 and moved it from draft to ready for review.

@milleruntime
Copy link
Contributor Author

I think the work that needed to be done in the code for this is done. So closing. There is a separate ticket for documentation that is still open #1416

@ctubbsii ctubbsii added this to the 2.1.0 milestone Jul 12, 2024
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

No branches or pull requests

3 participants