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

services/horizon: Relax requirement on Core URL flags #2780

Merged
merged 6 commits into from
Jul 3, 2020
Merged

services/horizon: Relax requirement on Core URL flags #2780

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e9da0e6
Relax requirement on Core URL flags
2opremio Jul 2, 2020
4469aac
Appease linter go vet
2opremio Jul 2, 2020
8902fd1
Address review feedback
2opremio Jul 3, 2020
3faaff2
Do not use the core DB session if captive is used
2opremio Jul 3, 2020
fc33126
Fix typo
2opremio Jul 3, 2020
a3bf665
Validate Core URL arguments for all app-based commands
2opremio Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions services/horizon/cmd/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,26 +204,30 @@ var dbReingestRangeCmd = &cobra.Command{

initRootConfig()

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}

horizonSession, err := db.Open("postgres", config.DatabaseURL)
if err != nil {
log.Fatalf("cannot open Horizon DB: %v", err)
}

ingestConfig := expingest.Config{
CoreSession: coreSession,
NetworkPassphrase: config.NetworkPassphrase,
HistorySession: horizonSession,
HistoryArchiveURL: config.HistoryArchiveURLs[0],
MaxReingestRetries: int(retries),
ReingesRetryBackoffSeconds: int(retryBackoffSeconds),
}

if config.EnableCaptiveCoreIngestion {
ingestConfig.StellarCorePath = config.StellarCoreBinaryPath
} else {
if config.StellarCoreDatabaseURL == "" {
log.Fatalf("flag --%s cannot be empty", stellarCoreDBURLFlagName)
}
Comment on lines +223 to +225
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with the current solution is that duplicates code. Another possibility would be to move this check inside initRootConfig and add a param that explains the config context (normal run, reingestion, verify-range, ...).

Copy link
Contributor Author

@2opremio 2opremio Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should worry too much about duplicated code since the Core dependency is going away medium-term.

Also, as I understand, the solution you propose needs to consider what command is being run, which is prone to error and handles command options far away from the command itself (I would rather do the check where the flag is used, for readibility).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the old database backend will be removed right after we move captive out of experimental mode. There are many Horizon deployments out there and they will definitely need time to migrate.

As for duplication, actually we don't need to add params. Checking StellarCoreDatabaseURL depends on EnableCaptiveCoreIngestion which is already present in initRootConfig. The only thing we need to check is StellarCoreDatabaseURL and we can actually do it in root command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for duplication, actually we don't need to add params. Checking StellarCoreDatabaseURL depends on EnableCaptiveCoreIngestion which is already present in initRootConfig. The only thing we need to check is StellarCoreDatabaseURL and we can actually do it in root command.

That's not really the case, because right now the root command (and serve) need to use the Core URLs when ingestion is enabled regardless of the value of EnableCaptiveCoreIngestion

Copy link
Contributor Author

@2opremio 2opremio Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the old database backend will be removed right after we move captive out of experimental mode.

I didn't assume that, I said it will be removed medium-term.

Copy link
Contributor

@bartekn bartekn Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't assume that, I said it will be removed medium-term.

I mean, the duplication wouldn't be a problem if we were to remove it in the next or following release but it's likely it will stay in the codebase for at least a couple months.

That's not really the case, because right now the root command (and serve) need to use the Core URLs when ingestion is enabled regardless of the value of EnableCaptiveCoreIngestion

There's a mistake in my comment I meant:

The only thing we need to check is StellarCoreURL and we can actually do it in root command.

Sorry! So to be more clear:

  1. We move all the duplicated StellarCoreDatabaseURL checks to initRootCommand because we need EnableCaptiveCoreIngestion and Ingest values for proper validation and they are already there. StellarCoreDatabaseURL right now is only used in ingestion (ledger backend).
  2. We only check for StellarCoreURL in webserver and ingestion (but not when reingesting because we don't update the cursor). So we just validate it in serve because this is when it's really needed.

Sorry if I'm missing something but doing this will remove 4 x if config.StellarCoreDatabaseURL == "" { ... } so I guess it's worth a try.

Giving 👍 because I'm leaving soon and don't want to block this if there's really a mistake in my thinking.

Copy link
Contributor Author

@2opremio 2opremio Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

1. We move all the duplicated `StellarCoreDatabaseURL` checks to `initRootCommand` because we need `EnableCaptiveCoreIngestion` and `Ingest` values for proper validation and they are already there. `StellarCoreDatabaseURL` right now is only used in ingestion (ledger backend).

But ... isn't StellarCoreDatabaseURL required for online ingestion even if EnableCaptiveCoreIngestion isn't defined?

Copy link
Contributor Author

@2opremio 2opremio Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK. That is solvable by enforcing StellarCoreDatabaseURL in the serve command.

But then ... horizon db reingest range and horizon expingest {stress-test, verify-range} need to check that StellarCoreDatabaseURL or CaptiveCore are enabled regardless of the value of Ingest (because isn't checked), which isn't the case for the rest of the commands.

I don't see a way to to generalize it in the root initialization without checking what command is invoked.

Copy link
Contributor Author

@2opremio 2opremio Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So, I think I will merge as is. If you think you can generalize it in an elegant way without breaking existing behavior, I will be more than happy to change it.

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}
ingestConfig.CoreSession = coreSession
}

if parallelWorkers < 2 {
Expand Down
45 changes: 27 additions & 18 deletions services/horizon/cmd/ingest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,31 +24,31 @@ var ingestVerifyFrom, ingestVerifyTo, ingestVerifyDebugServerPort uint32
var ingestVerifyState bool

var ingestVerifyRangeCmdOpts = []*support.ConfigOption{
&support.ConfigOption{
{
Name: "from",
ConfigKey: &ingestVerifyFrom,
OptType: types.Uint32,
Required: true,
FlagDefault: uint32(0),
Usage: "first ledger of the range to ingest",
},
&support.ConfigOption{
{
Name: "to",
ConfigKey: &ingestVerifyTo,
OptType: types.Uint32,
Required: true,
FlagDefault: uint32(0),
Usage: "last ledger of the range to ingest",
},
&support.ConfigOption{
{
Name: "verify-state",
ConfigKey: &ingestVerifyState,
OptType: types.Bool,
Required: false,
FlagDefault: false,
Usage: "[optional] verifies state at the last ledger of the range when true",
},
&support.ConfigOption{
{
Name: "debug-server-port",
ConfigKey: &ingestVerifyDebugServerPort,
OptType: types.Uint32,
Expand Down Expand Up @@ -83,11 +83,6 @@ var ingestVerifyRangeCmd = &cobra.Command{
}()
}

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}

horizonSession, err := db.Open("postgres", config.DatabaseURL)
if err != nil {
log.Fatalf("cannot open Horizon DB: %v", err)
Expand All @@ -102,13 +97,22 @@ var ingestVerifyRangeCmd = &cobra.Command{
}

ingestConfig := expingest.Config{
CoreSession: coreSession,
NetworkPassphrase: config.NetworkPassphrase,
HistorySession: horizonSession,
HistoryArchiveURL: config.HistoryArchiveURLs[0],
}
if config.EnableCaptiveCoreIngestion {
ingestConfig.StellarCorePath = config.StellarCoreBinaryPath
} else {
if config.StellarCoreDatabaseURL == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

@2opremio is there maybe a way to specify conditional requirements in cobra? Like, this is only required if EnableCaptiveCoreIngestion is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abuiles something like this is done in initRootConfig, we already have a similar code there:

	if config.EnableCaptiveCoreIngestion && config.StellarCoreBinaryPath == "" {
		stdLog.Fatalf("--stellar-core-binary-path must be set when --enable-captive-core-ingestion is set")
	}

log.Fatalf("flag --%s cannot be empty", stellarCoreDBURLFlagName)
}

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}
ingestConfig.CoreSession = coreSession
}

system, err := expingest.NewSystem(ingestConfig)
Expand All @@ -132,15 +136,15 @@ var ingestVerifyRangeCmd = &cobra.Command{
var stressTestNumTransactions, stressTestChangesPerTransaction int

var stressTestCmdOpts = []*support.ConfigOption{
&support.ConfigOption{
{
Name: "transactions",
ConfigKey: &stressTestNumTransactions,
OptType: types.Int,
Required: false,
FlagDefault: int(1000),
Usage: "total number of transactions to ingest (at most 1000)",
},
&support.ConfigOption{
{
Name: "changes",
ConfigKey: &stressTestChangesPerTransaction,
OptType: types.Int,
Expand All @@ -162,11 +166,6 @@ var ingestStressTestCmd = &cobra.Command{

initRootConfig()

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}

horizonSession, err := db.Open("postgres", config.DatabaseURL)
if err != nil {
log.Fatalf("cannot open Horizon DB: %v", err)
Expand All @@ -181,13 +180,23 @@ var ingestStressTestCmd = &cobra.Command{
}

ingestConfig := expingest.Config{
CoreSession: coreSession,
NetworkPassphrase: config.NetworkPassphrase,
HistorySession: horizonSession,
HistoryArchiveURL: config.HistoryArchiveURLs[0],
}

if config.EnableCaptiveCoreIngestion {
ingestConfig.StellarCorePath = config.StellarCoreBinaryPath
} else {
if config.StellarCoreDatabaseURL == "" {
log.Fatalf("flag --%s cannot be empty", stellarCoreDBURLFlagName)
}

coreSession, err := db.Open("postgres", config.StellarCoreDatabaseURL)
if err != nil {
log.Fatalf("cannot open Core DB: %v", err)
}
ingestConfig.CoreSession = coreSession
}

system, err := expingest.NewSystem(ingestConfig)
Expand Down
22 changes: 16 additions & 6 deletions services/horizon/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ import (
"github.com/stellar/throttled"
)

const (
maxDBPingAttempts = 30
stellarCoreDBURLFlagName = "stellar-core-db-url"
stellarCoreURLFlagName = "stellar-core-url"
)

var (
config horizon.Config

Expand All @@ -28,13 +34,19 @@ var (
Short: "client-facing api server for the stellar network",
Long: "client-facing api server for the stellar network. It acts as the interface between Stellar Core and applications that want to access the Stellar network. It allows you to submit transactions to the network, check the status of accounts, subscribe to event streams and more.",
Run: func(cmd *cobra.Command, args []string) {
if config.Ingest {
if config.StellarCoreURL == "" {
2opremio marked this conversation as resolved.
Show resolved Hide resolved
log.Fatalf("flag --%s cannot be empty", stellarCoreDBURLFlagName)
}
if config.StellarCoreDatabaseURL == "" {
log.Fatalf("flag --%s cannot be empty", stellarCoreURLFlagName)
}
}
initApp().Serve()
},
}
)

const maxDBPingAttempts = 30

// validateBothOrNeither ensures that both options are provided, if either is provided.
func validateBothOrNeither(option1, option2 string) {
arg1, arg2 := viper.GetString(option1), viper.GetString(option2)
Expand Down Expand Up @@ -127,18 +139,16 @@ var configOpts = support.ConfigOptions{
ConfigKey: &config.EnableCaptiveCoreIngestion,
},
&support.ConfigOption{
Name: "stellar-core-db-url",
Name: stellarCoreDBURLFlagName,
EnvVar: "STELLAR_CORE_DATABASE_URL",
ConfigKey: &config.StellarCoreDatabaseURL,
OptType: types.String,
Required: true,
Usage: "stellar-core postgres database to connect with",
},
&support.ConfigOption{
Name: "stellar-core-url",
Name: stellarCoreURLFlagName,
ConfigKey: &config.StellarCoreURL,
OptType: types.String,
Required: true,
Usage: "stellar-core to connect with (for http commands)",
},
&support.ConfigOption{
Expand Down