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

2opremio · 2020-07-02T15:10:50Z

They shouldn't be required when captive core is used

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
[ x This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've updated the relevant CHANGELOG (here for Horizon) if
needed with deprecations, added features, breaking changes, and DB schema changes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

Only require --stellar-core-db-url and --stellar-core-url explicitly when they are really needed.

Fixes #2779

Why

An external Core instance is not required when using Captive Core, imposing artificial restrictions

Known limitations

The message printed out won't be identical to the one printed by Cobra.

They shouldn't be required when captive core is used

bartekn · 2020-07-02T15:25:03Z

services/horizon/cmd/db.go

+			if config.StellarCoreDatabaseURL == "" {
+				log.Fatalf("flag --%s cannot be empty", stellarCoreDBURLFlagName)
+			}


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, ...).

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).

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.

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

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.

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:

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).

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.

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?

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.

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.

abuiles · 2020-07-02T15:27:16Z

services/horizon/cmd/ingest.go

 			NetworkPassphrase: config.NetworkPassphrase,
 			HistorySession:    horizonSession,
 			HistoryArchiveURL: config.HistoryArchiveURLs[0],
 		}
 		if config.EnableCaptiveCoreIngestion {
 			ingestConfig.StellarCorePath = config.StellarCoreBinaryPath
+		} else {
+			if config.StellarCoreDatabaseURL == "" {


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

@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") }

services/horizon/cmd/root.go

bartekn

👍 but please check my last comment about duplication and #2780 (comment).

2opremio · 2020-07-03T13:07:23Z

Only require --stellar-core-db-url and --stellar-core-url explicitly when they are really needed.

An external Core instance is not required when using Captive Core, imposing artificial restrictions

Relax requirement on Core URL flags

e9da0e6

They shouldn't be required when captive core is used

2opremio requested a review from a team July 2, 2020 15:10

cla-bot bot added the cla: yes label Jul 2, 2020

2opremio mentioned this pull request Jul 2, 2020

services/horizon: Remove db2/core. #2759

Merged

7 tasks

bartekn reviewed Jul 2, 2020

View reviewed changes

abuiles reviewed Jul 2, 2020

View reviewed changes

Appease linter go vet

4469aac

bartekn reviewed Jul 2, 2020

View reviewed changes

services/horizon/cmd/root.go Outdated Show resolved Hide resolved

bartekn approved these changes Jul 2, 2020

View reviewed changes

2opremio added 4 commits July 3, 2020 12:51

Address review feedback

8902fd1

Do not use the core DB session if captive is used

3faaff2

Fix typo

fc33126

Validate Core URL arguments for all app-based commands

a3bf665

2opremio merged commit 457958e into release-horizon-v1.6.0 Jul 3, 2020

2opremio deleted the relax-core-cmd-args branch July 3, 2020 13:14

This was referenced Jul 3, 2020

Remove --stellar-core-db-url requirement when webserver is not running #2779

Closed

Ingesting from captive core should not require stellar-core-db-url or stellar-core-url #2641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

2opremio commented Jul 2, 2020 •

edited

Loading

bartekn Jul 2, 2020

2opremio Jul 2, 2020 •

edited

Loading

bartekn Jul 2, 2020

2opremio Jul 2, 2020

2opremio Jul 2, 2020 •

edited

Loading

bartekn Jul 2, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading

abuiles Jul 2, 2020

bartekn Jul 2, 2020

bartekn left a comment

2opremio commented Jul 3, 2020

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

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

Conversation

2opremio commented Jul 2, 2020 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

bartekn Jul 2, 2020

Choose a reason for hiding this comment

2opremio Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

bartekn Jul 2, 2020

Choose a reason for hiding this comment

2opremio Jul 2, 2020

Choose a reason for hiding this comment

2opremio Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

bartekn Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

2opremio Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

2opremio Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

2opremio Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

abuiles Jul 2, 2020

Choose a reason for hiding this comment

bartekn Jul 2, 2020

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

2opremio commented Jul 3, 2020

2opremio commented Jul 2, 2020 •

edited

Loading

2opremio Jul 2, 2020 •

edited

Loading

2opremio Jul 2, 2020 •

edited

Loading

bartekn Jul 2, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading

2opremio Jul 3, 2020 •

edited

Loading