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.

Sign up for GitHub

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

Jump to bottom

Add pool DB operation removeRetiredPools. #2047

Merged
merged 19 commits into from
Aug 24, 2020
Merged

Add pool DB operation removeRetiredPools. #2047

merged 19 commits into from
Aug 24, 2020

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Aug 18, 2020 •
edited
Loading

Issue Number

#2018

Overview

Building on the work added in PRs #2024 and #2038, this PR:

  • Adds a removeRetiredPools operation to the pool DB layer:

    removeRetiredPools  
    :: EpochNo
    -> stm [PoolRetirementCertificate]
    -- ^ Remove all pools with an active retirement epoch that is earlier 
    -- than or equal to the specified epoch.  
    --
    -- Returns the retirement certificates of the pools that were removed.
  • Writes a bracketed MsgRemovingRetiredPools message to the log whenever the removeRetiredPools operation is called:

    Looking for pools that retired in or before epoch 1000.
Removing the following retired pools:
    Pool 4355a46b with retirement epoch 999
    Pool 53c234e5 with retirement epoch 999
    Pool 121cfccd with retirement epoch 1000
    Pool 917df332 with retirement epoch 1000
Finished removing retired pools.

Further Work

Before garbage collection can actually happen, the wallet needs to call removeRetiredPools at appropriate intervals. This work is deferred to a future PR.

See issue #2019.

@jonathanknowles jonathanknowles self-assigned this Aug 18, 2020
@jonathanknowles jonathanknowles added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Aug 18, 2020
@jonathanknowles jonathanknowles requested a review from rvl August 18, 2020 05:25
rvl
rvl suggested changes Aug 18, 2020
View reviewed changes
lib/core/src/Cardano/Pool/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Pool/DB/Sqlite.hs Outdated Show resolved Hide resolved
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-pools branch from 8d5c7cb to b7636eb Compare August 18, 2020 06:36
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from 76d56dc to e048243 Compare August 18, 2020 06:45
jonathanknowles added a commit that referenced this pull request Aug 18, 2020
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
c912a26 
In response to review feedback here:

#2047 (comment)
@jonathanknowles jonathanknowles requested a review from rvl August 18, 2020 10:06
jonathanknowles added a commit that referenced this pull request Aug 18, 2020
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
e7810ad 
In response to review feedback here:

#2047 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from c912a26 to e7810ad Compare August 18, 2020 10:35
@jonathanknowles

This comment has been minimized.

Sign in to view
iohk-bors bot added a commit that referenced this pull request Aug 18, 2020
@iohk-bors
Try #2047:
4ce201b
@iohk-bors

This comment has been minimized.

Sign in to view
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-pools branch from b7636eb to 6039042 Compare August 18, 2020 12:13
jonathanknowles added a commit that referenced this pull request Aug 18, 2020
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
5b6f5af 
In response to review feedback here:

#2047 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from e7810ad to 5b6f5af Compare August 18, 2020 12:14
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-pools branch 2 times, most recently from 0741d66 to b5f1579 Compare August 19, 2020 05:51
@jonathanknowles jonathanknowles removed the request for review from rvl August 19, 2020 05:54
@jonathanknowles jonathanknowles marked this pull request as draft August 19, 2020 05:55
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-pools branch 3 times, most recently from cb87439 to 70a3cfd Compare August 20, 2020 04:19
Base automatically changed from jonathanknowles/remove-pools to master August 20, 2020 06:32
jonathanknowles added a commit that referenced this pull request Aug 20, 2020
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
55349a2 
In response to review feedback here:

#2047 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from 5b6f5af to 83baaae Compare August 20, 2020 08:03
jonathanknowles added a commit that referenced this pull request Aug 20, 2020
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
46b38aa 
In response to review feedback here:

#2047 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from 83baaae to ed4a446 Compare August 20, 2020 08:04
rvl
rvl approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Two comments about logging for you to address. Since removeRetiredPools is not used anywhere I assume there will be another PR.

lib/core/src/Cardano/Pool/DB/Log.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Pool/DB/Log.hs Show resolved Hide resolved
@jonathanknowles
Copy link
Contributor Author

Since removeRetiredPools is not used anywhere I assume there will be another PR.

That's correct. From the PR description:

Before garbage collection can actually happen, the wallet needs to call removeRetiredPools at appropriate intervals. This work is deferred to a future PR. See issue #2019.

@jonathanknowles
Revise Buildable instance for PoolRetirementCertificate.
08b4040 
Grammatically, the generated text now makes sense regardless of whether
the retirement is in the future or the past.
@jonathanknowles
Rename retiredIn to retirementEpoch.
a318d5b 
Rename `retiredIn` to `retirementEpoch` in `PoolRetirementCertificate`.

The phrase "retired in" is somewhat ambiguous, as it might lead readers
to believe that a pool has already retired, when in fact this value can
refer to an epoch that is still in the future.

In comparison, the phrase "retirement epoch" is neutral w.r.t. whether
the epoch is in the future or the past.
@jonathanknowles
Provide removeRetiredPools DB operation.
a0812e5
@jonathanknowles
Add MsgGarbageCollection logging constructor.
b5b2859
@jonathanknowles
Add logging to removeRetiredPools.
2e62689
@jonathanknowles
Make removeRetiredPools backend-agnostic.
f6ed991
@jonathanknowles
Introduce MsgGarbageCollectionStep.
aed7fda 
In preparation for bracket-style logging.
@jonathanknowles
Use bracket-style logging in removeRetiredPools.
ec3ddb9 
In response to review feedback here:

#2047 (comment)
@jonathanknowles
Add MsgRemovingRetiredPools constructor to PoolDbLog type.
49af89f
@jonathanknowles
Move PoolDbLog to separate module.
5f169aa 
To avoid cyclic module dependencies.
@jonathanknowles
Use PoolDbLog in removeRetiredPools.
b6211b1
@jonathanknowles
Use [PoolRetirementCertificate] in MsgRemovingRetiredPools.
b12c2d1
@jonathanknowles
Use MsgRemovingRetiredPools in removeRetiredPools.
75c0043
@jonathanknowles
Add constructor MsgRemovingRetiredPoolsForEpoch to PoolDbLog.
99ecf7b
@jonathanknowles
Use MsgRemovingRetiredPoolsForEpoch in removeRetiredPools.
5573a9a
@jonathanknowles
Removing unused MsgGarbageCollection[Step] constructors.
41caa64
@jonathanknowles
Remove unused LANGUAGE pragma.
118d22d
@jonathanknowles
List pools and remove them in a single atomic operation.
0465790 
In response to review feedback:

#2047 (comment)
@jonathanknowles
Reduce log Severity to Debug for pool retirement messages.
024d36b 
For:

- MsgRemovingRetiredPools
- MsgRemovingRetiredPoolsForEpoch

In response to review feedback:

#2047 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/remove-retired-pools branch from 5f9c7df to 024d36b Compare August 24, 2020 10:12
@jonathanknowles
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 24, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 90c1e59 into master Aug 24, 2020
@iohk-bors iohk-bors bot deleted the jonathanknowles/remove-retired-pools branch August 24, 2020 11:19
@jonathanknowles jonathanknowles mentioned this pull request Aug 25, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

Assignees

@jonathanknowles jonathanknowles

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
(ADP-376) Garbage collection of retir...
Development

Successfully merging this pull request may close these issues.
2 participants
@jonathanknowles @rvl