Add pool DB operation `removePools` #2038

jonathanknowles · 2020-08-13T10:46:30Z

Issue Number

#2017

Overview

This PR:

Adds a removePools operation to the pool DB layer:

removePools
    :: [PoolId]
    -> stm ()
    -- ^ Remove all data relating to the specified pools.

Provides the following implementations of removePools:
- Pool.DB.MVar
- Pool.DB.SQLite
Writes a MsgRemovingPool message to the log whenever a pool is removed:
```
Removing the following pool from the database: XXXX.
```
Adds a property test to check that only the specified pools are removed, and no other pools.

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

lib/core/src/Cardano/DB/Sqlite.hs

rvl · 2020-08-18T06:09:21Z

lib/core/src/Cardano/DB/Sqlite.hs

@@ -372,6 +372,7 @@ data DBLog
    | MsgWaitingForDatabase Text (Maybe Int)
    | MsgRemovingInUse Text Int
    | MsgRemoving Text
+    | MsgRemovingDatabaseEntity Text


How about being more specific?
MsgRemovePool PoolId

How about being more specific?
MsgRemovePool PoolId

I do agree that we should use specific types, wherever possible, rather than Text.

However, one issue is that DBLog type is general to all databases. Ideally, DBLog shouldn't use types that are specific to pools or wallets.

An obvious solution would be to introduce WalletDbLog and PoolDbLog types, of which the latter would have the constructor that you suggest: MsgRemovePool.

However, this would require changing references to DBLog in quite a few places. If we decide to do this, I suggest we create a separate PR (or make an item of technical debt), in order to avoid complicating this one too much.

What do you think?

I've created a technical debt ticket to cover the idea of defining separate WalletDbLog and PoolDbLog types here: https://jira.iohk.io/browse/ADP-407.

While tackling the above ticket, we could also go through the existing DbLog constructors and replace the "stringly-typed" constructors with strongly-typed equivalents in WalletDbLog and PoolDbLog. Hopefully, this should also clean up parts of the code that are obscured with log formatting logic.

I would like this to be fixed now, rather than kicking the can down the road.

@rvl wrote:

I would like this to be fixed now, rather than kicking the can down the road.

It wasn't clear to me that your original feedback wasn't just a suggestion:

How about being more specific?
MsgRemovePool PoolId

I agree that this would be better, but IMO fixing the lack of type specialization in DBLog is really orthogonal to the purpose of this PR, whose main focus is the removal of pools from the database. That the log messages use Text rather than PoolId seems like a minor issue, and not really a valid reason to delay the whole PR. (The other constructors in this type also use Text, so it's not inconsistent with the original code.)

IMO, it would be better to make these changes in separate PRs:

PR A: Implement the removePools operation.

PR B: Divide DBLog into PoolDbLog and WalletDbLog, and convert the various "stringly-typed" constructors within DBLog into strongly-typed constructors.

Indeed, we already had a ticket in place to perform the refactoring here https://jira.iohk.io/browse/ADP-407

But by attempting to do the refactoring in this PR, we risk adding extra delay, as any lack of consensus about how to perform the refactoring will end up delaying the PR as a whole.

Nevertheless, I've added the following commits:

bcec1b9 (Provide database-schema-specific logging types.)

44a72ec (Introduce log constructor MsgRemovingPool.)

8e5449f (Use MsgRemovingPool in removePools operation.)

b5f1579 (Remove unused MsgRemovingDatabaseEntity log constructor.)

Could have a look at these commits and let me know your thoughts?

Thanks!

lib/core/test/unit/Cardano/Pool/DB/Arbitrary.hs

Anviking

lgtm!

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

rvl

For the logging changes - thanks for trying to fix it.
I would go with something simpler.
Create a PoolDBLog data type which embeds DBLog plus your new log messages.
Then all the changes will be local to the Pool.DB.Sqlite module, and we don't need a DBLog schema type parameter everywhere.

In response to review feedback: #2038 (comment) #2038 (review)

jonathanknowles · 2020-08-20T04:22:32Z

Create a PoolDBLog data type which embeds DBLog plus your new log messages.
Then all the changes will be local to the Pool.DB.Sqlite module, and we don't need a DBLog schema type parameter everywhere.

Thanks for this suggestion. I've changed the types as you suggest.

See commits from 82be504 onwards.

rvl

Thanks!

jonathanknowles · 2020-08-20T05:39:35Z

bors r+

iohk-bors · 2020-08-20T06:32:17Z

Build succeeded

2047: Add pool DB operation `removeRetiredPools`. r=jonathanknowles a=jonathanknowles # Issue Number #2018 # Overview Building on the work added in PRs #2024 and #2038, this PR: - [x] Adds a `removeRetiredPools` operation to the pool DB layer:<br><br> ```hs 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. ``` - [x] Writes a bracketed `MsgRemovingRetiredPools` message to the log whenever the `removeRetiredPools` operation is called:<br><br> ``` 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. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

jonathanknowles self-assigned this Aug 13, 2020

jonathanknowles added this to the (ADP-376) Garbage collection of retired pools from the DB milestone Aug 13, 2020

jonathanknowles force-pushed the jonathanknowles/remove-pools branch 3 times, most recently from b0eb33a to 8461a48 Compare August 17, 2020 04:06

jonathanknowles marked this pull request as ready for review August 17, 2020 04:06

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from 8461a48 to c36325b Compare August 17, 2020 04:12

jonathanknowles requested a review from rvl August 17, 2020 04:12

jonathanknowles added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Aug 17, 2020

jonathanknowles requested a review from KtorZ August 17, 2020 07:49

jonathanknowles force-pushed the jonathanknowles/remove-pools branch 2 times, most recently from ae765e8 to 6447bf7 Compare August 17, 2020 09:47

piotr-iohk mentioned this pull request Aug 17, 2020

Add pool DB operation removePools. #2017

Closed

5 tasks

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from 6447bf7 to 8d5c7cb Compare August 18, 2020 03:44

jonathanknowles mentioned this pull request Aug 18, 2020

Add pool DB operation removeRetiredPools. #2047

Merged

2 tasks

jonathanknowles added this to QA in Adrestia Aug 18, 2020

rvl suggested changes Aug 18, 2020

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from 8d5c7cb to b7636eb Compare August 18, 2020 06:36

jonathanknowles requested a review from rvl August 18, 2020 06:40

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from b7636eb to 6039042 Compare August 18, 2020 12:13

Anviking reviewed Aug 18, 2020

View reviewed changes

lib/core/test/unit/Cardano/Pool/DB/Arbitrary.hs Show resolved Hide resolved

jonathanknowles requested a review from Anviking August 18, 2020 14:32

Anviking approved these changes Aug 18, 2020

View reviewed changes

lib/core/test/unit/Cardano/Pool/DB/Properties.hs Show resolved Hide resolved

jonathanknowles removed request for rvl and KtorZ August 18, 2020 15:52

cardano-foundation deleted a comment from iohk-bors bot Aug 18, 2020

jonathanknowles requested a review from rvl August 18, 2020 15:55

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Aug 19, 2020

Try #2038:

03699c3

rvl reviewed Aug 19, 2020

View reviewed changes

jonathanknowles added a commit that referenced this pull request Aug 20, 2020

Introduce type PoolDbLog.

5d016da

In response to review feedback: #2038 (comment) #2038 (review)

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from bfcc8e8 to cb87439 Compare August 20, 2020 04:18

jonathanknowles added 16 commits August 20, 2020 04:19

Add skeleton removePools operation to the pool DB.

b7dc0e4

Provide model implementation of removePools pool DB operation.

7a15b9a

Provide SQLite implementation of removePools pool DB operation.

8f3a0a4

Add new constructor MsgRemovingDatabaseEntity to type DBLog.

c3e4f69

Add logging to SQLite removePools DB operation.

7e6559a

Add property prop_removePools.

1ce8203

Improve shrinking for PoolId.

55ffce4

Improve shrinking for PoolRegistrationCertificate.

8f965a3

Improve shrinking for PoolCertificate.

e04ea62

Add a roundtrip textual encoding test for PoolId.

6a96195

Appease the hlint god.

14500d1

Introduce type PoolDbLog.

82be504

In response to review feedback: #2038 (comment) #2038 (review)

Introduce MsgRemovingPool constructor of PoolDbLog.

d0a6ed5

Use MsgRemovingPool in removePools DB operation.

db056e5

Remove unused constructor MsgRemovingDatabaseEntity.

68e9493

Use contramap to simplify nested tracing.

70a3cfd

jonathanknowles force-pushed the jonathanknowles/remove-pools branch from cb87439 to 70a3cfd Compare August 20, 2020 04:19

jonathanknowles requested a review from rvl August 20, 2020 04:22

rvl approved these changes Aug 20, 2020

View reviewed changes

iohk-bors bot merged commit 86c1eb8 into master Aug 20, 2020

iohk-bors bot deleted the jonathanknowles/remove-pools branch August 20, 2020 06:32

piotr-iohk moved this from QA to Closed in Adrestia Aug 24, 2020

CharlesMorgan-IOHK removed this from Closed in Adrestia Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pool DB operation `removePools` #2038

Add pool DB operation `removePools` #2038

jonathanknowles commented Aug 13, 2020 •

edited

Loading

rvl Aug 18, 2020

jonathanknowles Aug 18, 2020 •

edited

Loading

jonathanknowles Aug 18, 2020

rvl Aug 18, 2020

jonathanknowles Aug 19, 2020 •

edited

Loading

Anviking left a comment

This comment has been minimized.

rvl left a comment

jonathanknowles commented Aug 20, 2020 •

edited

Loading

rvl left a comment

jonathanknowles commented Aug 20, 2020

iohk-bors bot commented Aug 20, 2020

Add pool DB operation removePools #2038

Add pool DB operation removePools #2038

Conversation

jonathanknowles commented Aug 13, 2020 • edited Loading

Issue Number

Overview

rvl Aug 18, 2020

Choose a reason for hiding this comment

jonathanknowles Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

jonathanknowles Aug 18, 2020

Choose a reason for hiding this comment

rvl Aug 18, 2020

Choose a reason for hiding this comment

jonathanknowles Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking left a comment

Choose a reason for hiding this comment

This comment has been minimized.

rvl left a comment

Choose a reason for hiding this comment

jonathanknowles commented Aug 20, 2020 • edited Loading

rvl left a comment

Choose a reason for hiding this comment

jonathanknowles commented Aug 20, 2020

iohk-bors bot commented Aug 20, 2020

Build succeeded

Add pool DB operation `removePools` #2038

Add pool DB operation `removePools` #2038

jonathanknowles commented Aug 13, 2020 •

edited

Loading

jonathanknowles Aug 18, 2020 •

edited

Loading

jonathanknowles Aug 19, 2020 •

edited

Loading

jonathanknowles commented Aug 20, 2020 •

edited

Loading