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

Remove expired upgrades #2272

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

sisuresh
Copy link
Contributor

@sisuresh sisuresh commented Sep 13, 2019

Description

Resolves #1661

This change removes pending upgrades if the current ledger close time has gone past the upgrade time by 12 hours.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v5.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@@ -1831,3 +1831,58 @@ TEST_CASE("upgrade invalid during ledger close", "[upgrades]")
REQUIRE_THROWS(executeUpgrade(*app, makeTxCountUpgrade(0)));
REQUIRE_THROWS(executeUpgrade(*app, makeBaseReserveUpgrade(0)));
}

void
Copy link
Contributor

Choose a reason for hiding this comment

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

static or use empty namespace for helper function

}
}

TEST_CASE("remove expired upgrades", "[upgrades]")
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use sections in this case: not because the setup is expensive but to avoid creating too many ultra small tests (when we run parallel tests, we split using test cases).
Note that using a section also allows (if you want) to move the helper function inside the test case (as a lambda), as to scope it better.

@MonsieurNicolas MonsieurNicolas added this to In progress in v12.1.0 via automation Sep 18, 2019
@MonsieurNicolas
Copy link
Contributor

looks good. squash and rebase and we'll merge when we open master for 12.1.0

@sisuresh sisuresh force-pushed the issue-1661-removeStaleUpgrades branch from 590603d to d5c6d12 Compare September 18, 2019 16:21
@sisuresh sisuresh force-pushed the issue-1661-removeStaleUpgrades branch from d5c6d12 to b1ff396 Compare September 18, 2019 16:27
@MonsieurNicolas
Copy link
Contributor

@sisuresh :
I just realized that this will break Stellar-commander (tests will not upgrade) - so we have to fix https://github.com/stellar/stellar_core_commander/blob/b68c06e8507924b338e15c3373d01c7e0b3205f1/lib/stellar_core_commander/process.rb#L350 to use now instead of epoch.

@MonsieurNicolas
Copy link
Contributor

r+ b1ff396

latobarita added a commit that referenced this pull request Oct 4, 2019
Remove expired upgrades

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit b1ff396 into stellar:master Oct 4, 2019
v12.1.0 automation moved this from In progress to Done Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v12.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

Remove stale pending upgrades
3 participants