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

Add ServiceRuntime#beforeCommit [ECR-3584] #1132

Conversation

dmitry-timofeev
Copy link
Contributor

@dmitry-timofeev dmitry-timofeev commented Sep 27, 2019

Overview

Add ServiceRuntime#beforeCommit.


See: https://jira.bf.local/browse/ECR-3584

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has Javadoc
  • Method preconditions are checked and documented in the Javadoc of the method
  • Changelog is updated if needed (in case of notable or breaking changes)
  • The continuous integration build passes

@@ -98,6 +98,21 @@ default void configure(Fork fork, Configuration configuration) {
*/
void createPublicApiHandlers(Node node, Router router);

/**
* Handles the changes made by all transactions included in the upcoming block.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alekseysidorov would you check please if the user-facing documentation is accurate?

*/
void createCheckpoint() {
public void createCheckpoint() {
// todo: (1) Are nested rollbacks supported, or is the existing checkpoint lost when a new one
Copy link
Contributor Author

Choose a reason for hiding this comment

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

❗️

Copy link
Contributor

Choose a reason for hiding this comment

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

Nested rollbacks are not possible, only one checkpoint is available at time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I submitted a task to consider to hide this operation, but I had initially misunderstood how it works and the possible consequences: as multiple rollbacks do not reset the state to the initial fork state, changes made by other services won't be affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new test and the updated documentation must hopefully clarify the things.

@coveralls
Copy link

coveralls commented Sep 27, 2019

Coverage Status

Coverage decreased (-0.1%) to 85.503% when pulling aed31b8 on dmitry-timofeev:add-before-commit-ECR-3584 into 4b56ff0 on exonum:dynamic-services.

Replace the mistakenly imported assertThat method with the main one.
@dmitry-timofeev dmitry-timofeev force-pushed the add-before-commit-ECR-3584 branch from b17f6dc to df7cb84 Compare September 30, 2019 08:43
Copy link
Contributor

@MakarovS MakarovS left a comment

Choose a reason for hiding this comment

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

👍

@dmitry-timofeev dmitry-timofeev merged commit 89768b2 into exonum:dynamic-services Oct 3, 2019
@dmitry-timofeev dmitry-timofeev deleted the add-before-commit-ECR-3584 branch October 3, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants