Skip to content

IGNITE-19119 Prepare org.apache.ignite.internal.storage.index.IndexStorage to built index#1839

Merged
tkalkirill merged 11 commits intoapache:mainfrom
gridgain:ignite-19119
Mar 28, 2023
Merged

IGNITE-19119 Prepare org.apache.ignite.internal.storage.index.IndexStorage to built index#1839
tkalkirill merged 11 commits intoapache:mainfrom
gridgain:ignite-19119

Conversation

@tkalkirill
Copy link
Contributor

*
* @throws StorageException If failed to get the row ID.
*/
@Nullable RowId getNextRowIdToBuild();
Copy link
Contributor

Choose a reason for hiding this comment

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

All other methods in this class declare throws StorageException explicitly

Copy link
Contributor Author

@tkalkirill tkalkirill Mar 27, 2023

Choose a reason for hiding this comment

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

But this exception is not checked, there is no need to specify it, documentation is enough.
I can remove it in other methods, but I think we should not change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this exception is not checked, there is no need to specify it, documentation is enough.

I agree with you, but consistency is more important. So we either add them everywhere or remove them. I think @ibessonov may be against removing them, because he added these throws in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my side - exception in the signature simply makes it more explicit.
StorageException should have been checked, and we wouldn't have this discussion in such case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed it personally, decided to remake StorageException into a checked in the future and therefore add it to the signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do that, checked exceptions suck and should not be used

@tkalkirill tkalkirill merged commit 37d1e0f into apache:main Mar 28, 2023
@tkalkirill tkalkirill deleted the ignite-19119 branch March 28, 2023 05:40
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
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.

3 participants