exp/ingest: Add processor to ingest ledger headers #1949

tamirms · 2019-11-19T11:40:13Z

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

This PR extends the experimental ingestion system to cover ledger headers. The database processor in the ledger pipeline will populate the history_ledgers table. The processor intends to insert the same history_ledgers rows as the services/horizon/internal/ingest package.

Why

The /ledgers and /ledgers/{ledger_id} endpoints serve data from the history_ledgers table. Currently, the table is populated by the legacy ingestion system. The experimental ingestion system should be populating the table if we want to migrate away from the legacy ingestion system.

Note that unlike other experimental ingestion processors, the ledgers processor writes to the same table as the legacy ingestion system. The ledger processor should be interchangeable with the legacy ingestion system in that they both produce identical rows in the history_ledgers table. The advantage of this approach is that we don't need to backfill old ledgers when we switch over to the experimental ingestion system.

Backfilling old ledgers is a relatively expensive operation because we need to process every single ledger in Stellar's history. With the other experimental ingestion processors, this was not a concern because it was possible to backfill all state using the history archive snapshot for a single ledger checkpoint.

Known limitations

When starting ingestion it is possible that the most recent history_ledgers table row is not consistent with historyQ.GetLastLedgerExpIngest(). Handling this problem requires some more thought and will be tackled in a separate PR.
The interplay between ledgerReaderWrapper and readerWrapperLedger is really confusing. I have tried to mimic the style and conventions of the existing codebase but I think we should get rid of these wrapper pipeline objects

abuiles

👍

abuiles · 2019-11-19T13:55:55Z

exp/ingest/io/ledger_reader_test.go

+		t.Fatalf("unexpected error %v", err)
+	}
+	backend.AssertExpectations(t)
+	if got := reader.CloseTime(); got != expectedCloseTime {


@tamirms is there a specific reason for not using Assert.Equal here?

no, I should have used Assert.Equal

tamirms · 2019-11-21T14:31:35Z

I think I want to go back on the decision to use the same table ( history_ledgers ) for both the legacy and experimental ingestion systems. Until the experimental ingestion system reaches parity with the legacy ingestion system we will need to run horizon with both systems enabled. That means both the legacy ingestion system and the experimental ingestion system will be writing to the same table. That will lead to errors when one of the ingestion systems tries to insert a ledger which is already present in the table.

To address this issue we could add an ON CONFLICT DO NOTHING clause to the insert statements in both ingestion systems. Or, we could add a configuration option for the legacy ingestion system to disable the ingestion of ledgers while the experimental ingestion system is running.

My concern with implementing one of those two solutions is that there might be some negative consequences if the history_ledgers table is not at a consistent ledger sequence number with the other legacy ingestion tables. I think there are several queries which join against the history_ledgers table.

I think the safest solution might be to configure the experimental ingestion system to write to another table exp_history_ledgers. But, we would continue to serve horizon requests using the history_ledgers table. Then, once we are ready to transition to using the experimental ingestion system instead of the legacy ingestion system, we can change the code to write to history_ledgers and discard exp_history_ledgers.

When running horizon with both the legacy and experimental ingestion systems enabled, we can have a routine which compares recently inserted rows in history_ledgers and exp_history_ledgers to check the columns match (ignoring created_at and updated_at because they depend on when the ingestion system becomes aware of a new ledger).

I think with this approach we reduce the risk of bugs while also verifying that the ingestion of ledgers in the experimental system is completely consistent with the legacy system.

bartekn · 2019-11-21T14:59:32Z

I think there should be a code that turns off old ingestion system when exp ingestion is enabled (when it has processors for all of existing historical data types). To ensure that new system works correctly we can reingest entire history and compare DB dumps of history_ tables. @tamirms idea makes sense too, I think the only disadvantage is we'll have to remember to update exp_history_* tables when history_* changes, ex. due to #1808.

bartekn

LGTM. I think it's possible to improve the code by moving code to different files.

exp/ingest/io/ledger_reader.go

exp/ingest/ledgerbackend/database_backend.go

exp/ingest/pipeline/main.go

exp/ingest/pipeline/wrappers.go

bartekn · 2019-11-21T15:08:10Z

services/horizon/internal/db2/history/ledger.go

+// LegacyIngestionVersion reflects the latest version of the non-experimental ingestion
+// algorithm. As rows are ingested into the horizon database, this version is
+// used to tag them.  In the future, any breaking changes introduced by a
+// developer should be accompanied by an increase in this value.
+//
+// Scripts, that have yet to be ported to this codebase can then be leveraged
+// to re-ingest old data with the new algorithm, providing a seamless
+// transition when the ingested data's structure changes.
+const LegacyIngestionVersion = 16


I was thinking about this today, I think we should keep a single version so update expingest.CurrentVersion to 16.

I don't think using the same version for both ingestion systems makes sense. If we need to bump the experimental ingestion version wouldn't that trigger re-ingeston on the legacy system?

Reingestion of historical data in the legacy system is manual (horizon db reingest ...).

bartekn · 2019-11-21T15:14:20Z

When starting ingestion it is possible that the most recent history_ledgers table row is not consistent with historyQ.GetLastLedgerExpIngest(). Handling this problem requires some more thought and will be tackled in a separate PR.

Possible solution here: #1969

bartekn · 2019-11-21T15:15:20Z

Forgot about one last thing: The base branch should be 0.25.0 or 0.26.0 whichever is planned to be released next year.

tamirms · 2019-11-21T15:28:16Z

I think there should be a code that turns off old ingestion system when exp ingestion is enabled (when it has processors for all of existing historical data types). To ensure that new system works correctly we can reingest entire history and compare DB dumps of history_ tables. @tamirms idea makes sense too, I think the only disadvantage is we'll have to remember to update exp_history_* tables when history_* changes, ex. due to #1808.

I don't like this approach because we cannot gradually release the remaining ingestion components. We will need to wait until all the historical data processors are ready and then do a large release which transitions between the old system and the new ingestion system.

tamirms · 2019-11-21T16:45:05Z

Forgot about one last thing: The base branch should be 0.25.0 or 0.26.0 whichever is planned to be released next year.

if we change the processor to write to exp_history_ledgers then we can release this in 0.24.0 right?

abuiles · 2019-11-21T22:06:41Z

@tamirms

When running horizon with both the legacy and experimental ingestion systems enabled, we can have a routine which compares recently inserted rows in history_ledgers and exp_history_ledgers to check the columns match (ignoring created_at and updated_at because they depend on when the ingestion system becomes aware of a new ledger).

I think we could do something similar to what @bartekn did on the /accounts/{id} end-point -- server from the old table but if ingestion is enabled, then compare that the values of history_ledgers and exp_history_ledgers match (minus created_at)

abuiles

👍

services/horizon/internal/db2/history/ledger.go

services/horizon/internal/db2/schema/migrations/26_exp_history_ledgers.sql

services/horizon/internal/expingest/processors/database_processor.go

services/horizon/internal/db2/history/ledger.go

Also, use same importer version as the legacy ingestion system

…sor.go Co-Authored-By: Bartek Nowotarski <[email protected]>

tamirms · 2019-11-25T18:37:49Z

@bartekn I have addressed the code review feedback, please take another look

bartekn

Just two issues connected to error reporting. The rest of the code LGTM!

bartekn · 2019-11-25T19:14:45Z

services/horizon/internal/expingest/processors/database_processor.go

+		return verify.NewStateError(errors.Errorf(
+			"No rows affected when ingesting new ledger: %v",
+			r.GetSequence(),
+		))


Similarly to the previous StateError issue we probably shouldn't mark state as invalid because of this.

Also, before the new table contents is used in actions we probably should return errors here because it will block meta ingestion. Instead, we probably need to log it directly like log.WithField("service", "expingest").Error(...).

bartekn · 2019-11-25T19:15:08Z

services/horizon/internal/expingest/processors/database_processor.go

+			"ledger %v in exp_history_ledgers does not match ledger in history_ledgers",
+			seq,
+		)
+	}


For two return errors above:

Before the new table contents is used in actions we probably should return errors here because it will block meta ingestion. Instead, we probably need to log it directly like log.WithField("service", "expingest").Error(...).

cla-bot bot added the cla: yes label Nov 19, 2019

tamirms changed the base branch from master to release-horizon-v0.24.0 November 19, 2019 11:40

tamirms force-pushed the ledger-processor branch from 0bf92b2 to 1bbd3ff Compare November 19, 2019 11:41

abuiles reviewed Nov 19, 2019

View reviewed changes

tamirms changed the title ~~Ledger processor~~ exp/ingest: Add processor to ingest ledger headers Nov 19, 2019

tamirms marked this pull request as ready for review November 19, 2019 20:20

tamirms requested review from ire-and-curses and bartekn November 19, 2019 20:21

bartekn reviewed Nov 21, 2019

View reviewed changes

tamirms force-pushed the ledger-processor branch 2 times, most recently from 02095e4 to fea685b Compare November 21, 2019 16:33

tamirms force-pushed the ledger-processor branch 2 times, most recently from bb8cdac to 0f6c3ba Compare November 21, 2019 21:02

abuiles approved these changes Nov 21, 2019

View reviewed changes

bartekn reviewed Nov 25, 2019

View reviewed changes

tamirms added 8 commits November 25, 2019 09:57

Experimental Ingestion for Ledgers

df89c27

Fix ledger reader wrapper

c77e725

Also, use same importer version as the legacy ingestion system

Add tests

1fa86ac

Fix test failures

7e5211a

use assert.Equal

174b6ac

Remove unnecessary ledger reader functions

8a79e1d

Add exp_history_ledgers migration

97e3e75

add QExpLedgers functions

a479baa

Compare exp_history_ledgers with history_ledgers

4bf793a

tamirms force-pushed the ledger-processor branch from 0f6c3ba to 29805bf Compare November 25, 2019 18:14

Keep version in expingest package

8826967

tamirms force-pushed the ledger-processor branch from 29805bf to 8826967 Compare November 25, 2019 18:15

tamirms and others added 3 commits November 25, 2019 10:22

Update services/horizon/internal/expingest/processors/database_proces…

01189b3

…sor.go Co-Authored-By: Bartek Nowotarski <[email protected]>

Fix CheckExpLedger typo

009d1b2

Use normal error to verify ledger rows

e110c8b

bartekn approved these changes Nov 25, 2019

View reviewed changes

Log consistency errors in ingestLedgerHeader

550297f

tamirms merged commit da07265 into release-horizon-v0.24.0 Nov 26, 2019

tamirms deleted the ledger-processor branch November 26, 2019 14:24

tamirms mentioned this pull request Nov 26, 2019

Implement ledger DB processor #1708

Closed

bartekn mentioned this pull request Jan 21, 2020

Implement ledger endpoints using experimental ingestion #1709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exp/ingest: Add processor to ingest ledger headers #1949

exp/ingest: Add processor to ingest ledger headers #1949

tamirms commented Nov 19, 2019 •

edited

Loading

abuiles left a comment

abuiles Nov 19, 2019

tamirms Nov 19, 2019

tamirms commented Nov 21, 2019

bartekn commented Nov 21, 2019 •

edited

Loading

bartekn left a comment

bartekn Nov 21, 2019

tamirms Nov 21, 2019

bartekn Nov 21, 2019

bartekn commented Nov 21, 2019

bartekn commented Nov 21, 2019

tamirms commented Nov 21, 2019

tamirms commented Nov 21, 2019

abuiles commented Nov 21, 2019

abuiles left a comment

tamirms commented Nov 25, 2019

bartekn left a comment

bartekn Nov 25, 2019

bartekn Nov 25, 2019

exp/ingest: Add processor to ingest ledger headers #1949

exp/ingest: Add processor to ingest ledger headers #1949

Conversation

tamirms commented Nov 19, 2019 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

abuiles left a comment

Choose a reason for hiding this comment

abuiles Nov 19, 2019

Choose a reason for hiding this comment

tamirms Nov 19, 2019

Choose a reason for hiding this comment

tamirms commented Nov 21, 2019

bartekn commented Nov 21, 2019 • edited Loading

bartekn left a comment

Choose a reason for hiding this comment

bartekn Nov 21, 2019

Choose a reason for hiding this comment

tamirms Nov 21, 2019

Choose a reason for hiding this comment

bartekn Nov 21, 2019

Choose a reason for hiding this comment

bartekn commented Nov 21, 2019

bartekn commented Nov 21, 2019

tamirms commented Nov 21, 2019

tamirms commented Nov 21, 2019

abuiles commented Nov 21, 2019

abuiles left a comment

Choose a reason for hiding this comment

tamirms commented Nov 25, 2019

bartekn left a comment

Choose a reason for hiding this comment

bartekn Nov 25, 2019

Choose a reason for hiding this comment

bartekn Nov 25, 2019

Choose a reason for hiding this comment

tamirms commented Nov 19, 2019 •

edited

Loading

bartekn commented Nov 21, 2019 •

edited

Loading