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

Replace nonpublic Accumulo IterationInterruptedException #2539

Open
wants to merge 11 commits into
base: integration
Choose a base branch
from

Conversation

SethSmucker
Copy link
Collaborator

IterationInterruptedException is not part of Accumulo's public API.

Create IterationInterruptedException class in DataWave, replacing Accumulo's version.

part of #2443

avgAGB
avgAGB previously approved these changes Aug 28, 2024
foster33
foster33 previously approved these changes Aug 28, 2024
/**
* Exception thrown if an interrupt flag is detected.
*/
public class IterationInterruptedException extends RuntimeException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might suggest customizing the name slightly to avoid IDE's pulling in accumulo's import of the class of the same name to avoid confusion later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@SethSmucker SethSmucker dismissed stale reviews from foster33 and avgAGB via 05db63f August 29, 2024 18:04
Copy link
Collaborator

@ivakegg ivakegg left a comment

Choose a reason for hiding this comment

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

See Adam's comment

@SethSmucker
Copy link
Collaborator Author

See Adam's comment

Is it still too close? I changed it from IterationInterruptedException to IterationInterruptException. Might be a bit confusing if they're this close, what do you think?

@ivakegg
Copy link
Collaborator

ivakegg commented Sep 10, 2024

See Adam's comment

Is it still too close? I changed it from IterationInterruptedException to IterationInterruptException. Might be a bit confusing if they're this close, what do you think?

Now that I am looking one layer deeper, I realize that we were throwing that exception because we are implementing InterruptibleIterator which is an accumulo class. Is that class part of the public API? If so then we need to push for the original IteratorInterruptedException to be part of the public api. Can you push that question over to the accumulo group please?

@ddanielr
Copy link
Collaborator

See Adam's comment

Is it still too close? I changed it from IterationInterruptedException to IterationInterruptException. Might be a bit confusing if they're this close, what do you think?

Now that I am looking one layer deeper, I realize that we were throwing that exception because we are implementing InterruptibleIterator which is an accumulo class. Is that class part of the public API? If so then we need to push for the original IteratorInterruptedException to be part of the public api. Can you push that question over to the accumulo group please?

@ivakegg the InterruptibleIterator is not in the list of iterators in the accumulo public API
After looking through the code, there are two datawave iterators which implement InterruptibleIterator, SortedMultiMapIterator and ColumnRangeIterator.

SortedMultiMapIterator is used for a test case so it could probably be modified to implement a public iterator class without too much impact to other code behavior. However ColumnRangeIterator might need further review to determine the next steps.

@ivakegg
Copy link
Collaborator

ivakegg commented Sep 16, 2024

Ok, it appears that whole mechanism is basically OBE. So essentially, everywhere we were passing InterationInterruptedException should remain as is. We simply should not be implementing the InterruptibleIterator and hence should not have any flag to check on. @SethSmucker Please move that whole mechanism, and keep the places where we are passing the InterationInterruptedException back up the stack.

@ivakegg
Copy link
Collaborator

ivakegg commented Oct 9, 2024

If you make the changes in your other PR, then I believe this one becomes OBE and can be closed. We still need to make sure that exception is thrown up the stack.

@ivakegg ivakegg added the MXTC label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants