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

Live channel database backup #951

Merged
merged 17 commits into from Apr 19, 2019
Merged
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -203,7 +203,7 @@
<dependency>
<groupId>org.xerial</groupId>
<artifactId>sqlite-jdbc</artifactId>
<version>3.21.0.1</version>
<version>3.27.2.1</version>
</dependency>
<dependency>
<!-- This is to get rid of '[WARNING] warning: Class javax.annotation.Nonnull not found - continuing with a stub.' compile errors -->
@@ -18,6 +18,14 @@ eclair {

watcher-type = "bitcoind" // other *experimental* values include "electrum"

// specific mail box for our backup handler, bounded to 1 message
// DO NOT overwrite these settings
backup-mailbox {
mailbox-type = "akka.dispatch.BoundedMailbox"
mailbox-capacity = 1
mailbox-push-timeout-time = 0
}

bitcoind {
host = "localhost"
rpcport = 18332
@@ -43,7 +43,7 @@ import fr.acinq.eclair.blockchain.fee.{ConstantFeeProvider, _}
import fr.acinq.eclair.blockchain.{EclairWallet, _}
import fr.acinq.eclair.channel.Register
import fr.acinq.eclair.crypto.LocalKeyManager
import fr.acinq.eclair.db.Databases
import fr.acinq.eclair.db.{BackupHandler, Databases}
import fr.acinq.eclair.io.{Authenticator, Server, Switchboard}
import fr.acinq.eclair.payment._
import fr.acinq.eclair.router._
@@ -220,11 +220,11 @@ class Setup(datadir: File,
routerTimeout = after(FiniteDuration(config.getDuration("router.init-timeout").getSeconds, TimeUnit.SECONDS), using = system.scheduler)(Future.failed(new RuntimeException("Router initialization timed out")))
_ <- Future.firstCompletedOf(routerInitialized.future :: routerTimeout :: Nil)

chaindir = new File(datadir, chain)
This conversation was marked as resolved by pm47

This comment has been minimized.

Copy link
@pm47

pm47 Apr 17, 2019

Member

There must be a better way to do this than to redefine the directory here. There was even a TODO comment that you missed when you moved it.

wallet = bitcoin match {
case Bitcoind(bitcoinClient) => new BitcoinCoreWallet(bitcoinClient)
case Electrum(electrumClient) =>
// TODO: DRY
val chaindir = new File(datadir, chain)
val sqlite = DriverManager.getConnection(s"jdbc:sqlite:${new File(chaindir, "wallet.sqlite")}")
val walletDb = new SqliteWalletDb(sqlite)
val electrumWallet = system.actorOf(ElectrumWallet.props(seed, electrumClient, ElectrumWallet.WalletParameters(nodeParams.chainHash, walletDb)), "electrum-wallet")
@@ -234,7 +234,14 @@ class Setup(datadir: File,
_ = wallet.getFinalAddress.map {
case address => logger.info(s"initial wallet address=$address")
}

backupHandler = system.actorOf(
SimpleSupervisor.props(
BackupHandler.props(
nodeParams.db,
new File(chaindir, "eclair.bak.wip"),
This conversation was marked as resolved by pm47

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
new File(chaindir, "eclair.bak.wip"),
new File(chaindir, "eclair.bak.tmp"),
new File(chaindir, "eclair.bak")
), "backup", SupervisorStrategy.Resume)
)
audit = system.actorOf(SimpleSupervisor.props(Auditor.props(nodeParams), "auditor", SupervisorStrategy.Resume))
paymentHandler = system.actorOf(SimpleSupervisor.props(config.getString("payment-handler") match {
case "local" => LocalPaymentHandler.props(nodeParams)
@@ -345,8 +352,11 @@ class Setup(datadir: File,

// @formatter:off
sealed trait Bitcoin

This conversation was marked as resolved by pm47

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member

Formatting issue

case class Bitcoind(bitcoinClient: BasicBitcoinJsonRPCClient) extends Bitcoin

case class Electrum(electrumClient: ActorRef) extends Bitcoin

// @formatter:on

case class Kit(nodeParams: NodeParams,
@@ -0,0 +1,46 @@
package fr.acinq.eclair.db

import java.io.File

import akka.actor.{Actor, ActorLogging, Props}
import fr.acinq.eclair.channel.ChannelPersisted


/**
* README !
* This actor will synchronously make a backup of the database it was initialized with whenever it receives
* a ChannelPersisted event.
* To avoid piling up messages and entering an endless backup loop, it is supposed to be used with a bounded mailbox
* with a single item:
*
* backup-mailbox {
This conversation was marked as resolved by pm47

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member

Isn't there a way to enforce this configuration? That seems a bit error prone.

Overall the pattern is pretty nice.

* mailbox-type = "akka.dispatch.BoundedMailbox"
* mailbox-capacity = 1
* mailbox-push-timeout-time = 0
* }
*
* Messages that cannot be processed will be sent to dead letters
*
* @param databases database to backup
* @param wip work-in-progress file
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
* @param wip work-in-progress file
* @param tmpFile temporary file
* @param destination destination file
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
* @param destination destination file
* @param backupFile final backup file
*/
class BackupHandler(databases: Databases, wip: File, destination: File) extends Actor with ActorLogging {
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
class BackupHandler(databases: Databases, wip: File, destination: File) extends Actor with ActorLogging {
class BackupHandler(databases: Databases, tmpFile: File, backupFile: File) extends Actor with ActorLogging {

// we listen to ChannelPersisted events, which will trigger a backup
context.system.eventStream.subscribe(self, classOf[ChannelPersisted])

def receive = {
case persisted: ChannelPersisted =>
val start = System.currentTimeMillis()
databases.backup(wip)
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
databases.backup(wip)
databases.backup(tmpFile)
val result = wip.renameTo(destination)
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
val result = wip.renameTo(destination)
val result = tmpFile.renameTo(backupFile)

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member

From javadoc:

Many aspects of the behavior of this method are inherently platform-dependent: The rename operation might not be able to move a file from one filesystem to another, it might not be atomic, and it might not succeed if a file with the destination abstract pathname already exists. The return value should always be checked to make sure that the rename operation was successful.

require(result, s"cannot rename $wip to $destination")
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
require(result, s"cannot rename $wip to $destination")
require(result, s"cannot rename $tmpFile to $backupFile")
val end = System.currentTimeMillis()
log.info(s" database backup triggered by ${persisted.channelId} to $destination done in ${end - start} ms")
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
log.info(s" database backup triggered by ${persisted.channelId} to $destination done in ${end - start} ms")
log.info(s"database backup triggered by channelId=${persisted.channelId} took ${end - start}ms")
}
}

object BackupHandler {
def props(databases: Databases, wip: File, destination: File) = Props(new BackupHandler(databases: Databases, wip: File, destination: File)).withMailbox("eclair.backup-mailbox")
This conversation was marked as resolved by sstone

This comment has been minimized.

Copy link
@pm47

pm47 Apr 18, 2019

Member
Suggested change
def props(databases: Databases, wip: File, destination: File) = Props(new BackupHandler(databases: Databases, wip: File, destination: File)).withMailbox("eclair.backup-mailbox")
def props(databases: Databases, tmpFile: File, backupFile: File) = Props(new BackupHandler(databases: Databases, wip: File, destination: File)).withMailbox("eclair.backup-mailbox")
}
@@ -19,6 +19,7 @@ trait Databases {

val pendingRelay: PendingRelayDb

def backup(file: File) : Unit
}

object Databases {
@@ -45,6 +46,12 @@ object Databases {
override val peers = new SqlitePeersDb(eclairJdbc)
override val payments = new SqlitePaymentsDb(eclairJdbc)
override val pendingRelay = new SqlitePendingRelayDb(eclairJdbc)
override def backup(file: File): Unit = {
This conversation was marked as resolved by pm47

This comment has been minimized.

Copy link
@pm47

pm47 Apr 17, 2019

Member

This API makes the assumption that we will only use a single file to store the databases, which is a bit strange.

SqliteUtils.using(eclairJdbc.createStatement()) {
statement => {
statement.executeUpdate(s"backup to ${file.getAbsolutePath}")
}
}
}
}

}
@@ -82,7 +82,7 @@ object SqliteUtils {
* Obtain an exclusive lock on a sqlite database. This is useful when we want to make sure that only one process
* accesses the database file (see https://www.sqlite.org/pragma.html).
*
* The lock will be kept until the database is closed, or if the locking mode is explicitely reset.
* The lock will be kept until the database is closed, or if the locking mode is explicitly reset.
*
* @param sqlite
*/
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.