Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Remove @SuppressFBWarnings("IT_NO_SUCH_ELEMENT") in InstallSnapshotRequests, and ensure spotbugs does not complain.

https://issues.apache.org/jira/browse/RATIS-2003

How was this patch tested?

CI:
https://github.com/adoroszlai/ratis/actions/runs/7562098256

@adoroszlai adoroszlai self-assigned this Jan 17, 2024
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@adoroszlai Thanks a lots for working on this!

Let's take this chance to move the iterator code to an inner class; see https://issues.apache.org/jira/secure/attachment/13066117/1016_review.patch

@adoroszlai adoroszlai marked this pull request as draft January 18, 2024 14:05
@adoroszlai adoroszlai marked this pull request as ready for review January 18, 2024 16:02
@adoroszlai
Copy link
Contributor Author

Let's take this chance to move the iterator code to an inner class

Thanks @szetszwo for the review. I agree that extracting to inner class improves the code.

There is one piece from the review patch that I omitted: extracting the FileChunkProto readFileChunk(FileInfo info) method. The rest of the refactoring is straightforward, but I don't think I understand the reason fileIndex++ can be incremented outside the if (chunk.getDone()) block.

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@szetszwo szetszwo merged commit 82f7b3b into apache:master Jan 18, 2024
@adoroszlai adoroszlai deleted the RATIS-2003 branch January 18, 2024 16:28
symious pushed a commit to symious/ratis that referenced this pull request Apr 4, 2024
SzyWilliam pushed a commit to SzyWilliam/ratis that referenced this pull request Jun 12, 2024
szetszwo pushed a commit to szetszwo/ratis that referenced this pull request Jun 16, 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

Successfully merging this pull request may close these issues.

2 participants