Skip to content

Conversation

@SammyVimes
Copy link
Contributor

@SammyVimes SammyVimes commented Aug 16, 2021

@SammyVimes SammyVimes force-pushed the ignite-15298 branch 2 times, most recently from fe17e44 to d91b3d8 Compare August 18, 2021 17:49
* @param interactedAfterSnapshot {@code true} whether raft group was interacted with after the snapshot operation.
* @return Closure.
*/
public abstract BooleanSupplier snapshotCheckClosure(JRaftServerImpl restarted, boolean interactedAfterSnapshot);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think that all these abstract methods are a sign of over-engineering. Do we really need an abstract class for these two tests? I think it is easier to copy-paste the test itself....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are basically the same, any change in the raft group API would require changes in two places

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but all these abstract methods show that they are not really the same, only the overall test scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think raft set up, stop and restart operations are too complicated to copy-paste them. I don't see how abstract tests can hurt us

@SammyVimes SammyVimes force-pushed the ignite-15298 branch 2 times, most recently from d17bd70 to 92362fe Compare August 24, 2021 07:27
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