[AD-347] Checkpoint & archive Acid DB periodically #366

Haskell-mouse · 2018-10-29T01:40:18Z

Description:

Now Ariadne periodically makes a checkpoint of AcidState DB,
moves old checkpoints to Archive path and removes old archives.
How often this action is performed and how many archives will be
stored in the path is defined by the wallet's configuration
and can be overwritten by command line arguments.

YT issue: https://issues.serokell.io/issue/AD-347

Checklist:

gromakovsky · 2018-10-30T15:58:10Z

PR description doesn't comply with PR template:

Description is empty, actual description is above it.
Issue link is broken.
Checkmarks are not set.

ariadne/cardano/src/Ariadne/Config/CLI.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

ariadne/cardano/src/Ariadne/Config/Wallet.hs

ariadne/cardano/src/Ariadne/Config/CLI.hs

ariadne/cardano/src/Ariadne/Config/Wallet.hs

ariadne/cardano/src/Ariadne/Config/CLI.hs

ariadne/cardano/src/Ariadne/Config/Wallet.hs

dniku · 2018-11-13T13:05:34Z

ariadne/cardano/src/Ariadne/MainTemplate.hs

    mkWalletFace :: (Doc -> IO ()) -> WalletFace
    walletInitAction :: IO ()
-    (mkWalletFace, walletInitAction) =
+    (mkWalletFace, walletInitAction, walletAction) =


This conflicts with #364. One of you will have to resolve some merge conflicts.

#364 has been merged.

ariadne/cardano/src/Ariadne/MainTemplate.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Kernel.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

dniku · 2018-11-17T16:40:04Z

ariadne/cardano/src/Ariadne/MainTemplate.hs

    mkWalletFace :: (Doc -> IO ()) -> WalletFace
    walletInitAction :: IO ()
-    (mkWalletFace, walletInitAction) =
+    (mkWalletFace, walletInitAction, walletAction) =


#364 has been merged.

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Kernel.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

gromakovsky · 2018-12-03T13:38:17Z

CI failed, btw.

Haskell-mouse · 2018-12-03T18:36:12Z

Fixed

docs/configuration.md

gromakovsky · 2018-12-03T18:53:11Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

    , pwlRollbackBlocks
          :: NewestFirst NE Blund
          -> m ()
+    , pwlCleanAcidDB


It's currently in the core API section, but it's not core API. I think it should be a new section.
And actually now I think it is better to make it part of passiveWalletLayerComponent.
buildComponent "Wallet DB cleaner" (async cleanupAcidState) uninterruptibleCancel.

Though @Pastafarianist may disagree, so I'd wait for his comments. I think the fact that wallet layer uses acid-state is its implementation detail and the necessity to run some helper worker is caused solely by this detail, so I like the idea of hiding it inside WalletLayer (and fortunately it's very easy to do, because ComponentM allows you to do equivalent of withAsync).

Totally agree!
I thought about the possibility of hiding Acid DB (and related stuff) behind wallet layer, just thought it can be done as a separate task.

We have some tasks about hiding acid DB behind wallet layer (e. g. AD-306 and AD-493), but they are about hiding stuff that is already visible, i. e. that wasn't hidden when it was added.
pwlCleanAcidDB is a new stuff, so it's better to hide it when it's added, not postpone it. It shouldn't be hard anyway. But let's see what @Pastafarianist thinks first.

@gromakovsky I agree. Cleanup is an implementation detail and certainly not a part of core API.

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

dniku

Mostly stylistic stuff, but the one about misindented comment is significant. Also, please resolve conflicts as many have accumulated.

dniku · 2019-01-02T14:44:55Z

ariadne/cardano/src/Ariadne/MainTemplate.hs

-        -- It is going to call msPutBackendErrorToUI and rethrow exception, if something goes wrong
-        linkUI serviceThread $ msPutBackendErrorToUI uiFace
-        uiAction
+        withAsync serviceAction $ \serviceThread -> do


Indentation is mismatched between this line and the previous line. I'm not certain why this block has to be indented 2 spaces further, but it doesn't seem to cause any bugs. Anyway, please either indent the comment or dedent the block.

dniku · 2019-01-02T14:46:19Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs


-import Control.Monad.Except
-import Data.Acid (Update)
+import Control.Monad.Except (Except, runExcept, throwError, withExcept)


Kudos for explicit imports.

dniku · 2019-01-02T14:47:49Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

+          void $ flip catchAny (\e -> putStrLn @Text $ "Got error while cleaning up archive: " <> show e) $ do
+              removed <- liftIO $ cleanupOldAcidArchives path numberOfStoredArchives
+              writeLog Debug $ "Removed " <> pretty removed <> " old archive files"
+          pass


Redundant pass?

dniku · 2019-01-02T14:48:46Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/DB/Util/AcidState.hs

+cleanupOldAcidArchives
+    :: FilePath  -- ^ path with stored files
+    -> Int    -- how many files should be stored
+    -> IO Int -- how many files have been deleted


Seems like this function is used only once, and hence it could go into the where block under cleanupAcidState.

dniku · 2019-01-02T14:50:22Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

    , pwlRollbackBlocks
          :: NewestFirst NE Blund
          -> m ()
+    , pwlCleanAcidDB


@gromakovsky I agree. Cleanup is an implementation detail and certainly not a part of core API.

dniku · 2019-01-02T14:52:51Z

ariadne/cardano/test/backend/Test/Spec/Fixture.hs

+    => ProtocolMagic
+    -> FilePath
+    -> (PassiveWalletLayer m -> Kernel.PassiveWallet -> IO a)
+    -> IO a


Due to a decision by @manpages, we are moving to 2-space indentation. This is going to become mandatory after SRK-119 is resolved. Furthermore, indentation in this signature is inconsistent with indentation in withLayerInMemoryStorage. For now, please indent all signatures in this file with 2 spaces.

gromakovsky · 2019-01-17T15:50:45Z

ariadne/cardano/src/Ariadne/MainTemplate.hs

 initializeEverything
    :: forall uiComponents uiFace uiLangFace.
-       (Components (AllComponents uiComponents), HasCallStack)
+       (Components (AllComponents uiComponents))


Why did you delete HasCallStack?

gromakovsky · 2019-01-17T15:53:19Z

ariadne/cardano/src/Ariadne/MainTemplate.hs

          -- Make custom link to service thread for UI apps.
          -- It is going to call msPutBackendErrorToUI and rethrow exception, if something goes wrong
          linkUI serviceThread $ msPutBackendErrorToUI uiFace
-          logDebug logging "Launching the UI..."


Why? To be honest, I don't understand why there are any changes in this file. Yes, some changes were necessary in a previous version, but now I think this file should remain intact.

Now ariadne periodically makes a checkpoint of AcidState DB, moves old checkpoints to `Archive` path and removes old archives. How often this action is performed and how many archives will be stored in the path is determined by the wallet's configuration and can be overwritten by command line arguments.

1) Use "Time Second" instead of Int in config. 2) Don't expose wallet from PassiveWalletLayer 3) Now the thread which archive Acid DB now is controlled from MainTemplate 4) Other small changes 5) Resolve conflicts with the current master.

This commit fixes Ariadne tests which use wallet config.

Removed one unused constraint tuple and fixed docs

Problem: acid DB cleanup cation should not be exposed outside of the passiveWallet API. Solution: Remove it from PassiveWalletLayer and now the thread which periodically cleans the DB is run when passiveWalletLayerComponent is created.

Added few changes due to review and rebasing to the current master Should be sqashed before merging to master.

Revert unnecessary changes in the file.

Haskell-mouse requested review from dniku and gromakovsky as code owners October 29, 2018 01:40

Haskell-mouse force-pushed the Haskell-mouse/ad-347-acid-state-is-used-improperly branch 3 times, most recently from b310601 to d616d32 Compare October 29, 2018 03:39

gromakovsky suggested changes Oct 30, 2018

View reviewed changes

ariadne/cardano/src/Ariadne/Config/CLI.hs Outdated Show resolved Hide resolved

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs Outdated Show resolved Hide resolved

ariadne/cardano/src/Ariadne/Config/Wallet.hs Outdated Show resolved Hide resolved

dniku reviewed Nov 13, 2018

View reviewed changes

dniku suggested changes Nov 17, 2018

View reviewed changes

dniku changed the title ~~[AD-347] Acid-state is used improperly~~ [AD-347] Checkpoint & archive acid-state periodically Nov 19, 2018

dniku changed the title ~~[AD-347] Checkpoint & archive acid-state periodically~~ [AD-347] Checkpoint & archive Acid DB periodically Nov 19, 2018

Haskell-mouse force-pushed the Haskell-mouse/ad-347-acid-state-is-used-improperly branch from d616d32 to 29f0229 Compare December 3, 2018 12:44

Haskell-mouse requested a review from artemohanjanyan as a code owner December 3, 2018 12:44

Haskell-mouse force-pushed the Haskell-mouse/ad-347-acid-state-is-used-improperly branch from 7c74c61 to 0f1b7de Compare December 3, 2018 17:01

gromakovsky reviewed Dec 3, 2018

View reviewed changes

gromakovsky removed the request for review from artemohanjanyan December 10, 2018 12:15

dniku suggested changes Jan 2, 2019

View reviewed changes

Haskell-mouse force-pushed the Haskell-mouse/ad-347-acid-state-is-used-improperly branch from c95c995 to 4cebbac Compare January 16, 2019 10:51

gromakovsky suggested changes Jan 17, 2019

View reviewed changes

Haskell-mouse added 8 commits January 26, 2019 00:02

[AD-347] Changes due to PR.

d6856ed

1) Use "Time Second" instead of Int in config. 2) Don't expose wallet from PassiveWalletLayer 3) Now the thread which archive Acid DB now is controlled from MainTemplate 4) Other small changes 5) Resolve conflicts with the current master.

[AD-347] Fix the tests.

9e12ac0

This commit fixes Ariadne tests which use wallet config.

[AD-347] Small changes due to the review

d0c900f

Removed one unused constraint tuple and fixed docs

[AD-347] Minor changes due to review.

c79f033

Added few changes due to review and rebasing to the current master Should be sqashed before merging to master.

[AD-347] Removed MonadMask redunant constrained

ff7f813

[AD-347] Revert changes in MainTemplate.hs

7d6cb7e

Revert unnecessary changes in the file.

Haskell-mouse force-pushed the Haskell-mouse/ad-347-acid-state-is-used-improperly branch from f03e5a8 to 7d6cb7e Compare January 26, 2019 10:40

[AD-347] Checkpoint & archive Acid DB periodically #366

Are you sure you want to change the base?

[AD-347] Checkpoint & archive Acid DB periodically #366

Uh oh!

Conversation

Haskell-mouse commented Oct 29, 2018 • edited by dniku Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gromakovsky commented Oct 30, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gromakovsky commented Dec 3, 2018

Uh oh!

Haskell-mouse commented Dec 3, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dniku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Haskell-mouse commented Oct 29, 2018 •

edited by dniku

Loading