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

Bug 2226 fast replay streaming txmeta #2343

Merged

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Nov 15, 2019

Description

First cut at #2226. This has 3 main pieces:

  • A new XDR type of high-resolution metadata called LedgerCloseMeta that captures a per-txn, per-ledger-entry change stream that's equivalent to / a superset of the history tables currently being written into the core database for horizon's benefit.
  • A new "application mode" field in ApplicationImpl that lets us switch-off subsystems and/or replace them with in-memory stubs when running in modes that don't use them.
  • A new catchup option --replay-in-memory that runs catchup in application mode REPLAY_HISTORY_FOR_METADATA, that has no overlay or database, and uses a "stub" LedgerTxnRoot that returns empty for any query, and a "never committed" pseudo-root LedgerTxn connected to it, that we use as the in-memory ledger.

The idea here is for horizon to write a config file that mentions a local stream and then runs stellar-core as a subprocess, in either bulk replay mode (catchup --replay-in-memory) during initial ingestion or in run mode to track the ongoing changes in the network.

(cc @ire-and-curses and @bartekn)

RUN_LIVE_NODE,

// Application will connect to networks but _not_ establish a database
// of any sort, nor participate in consensus, apply transactions or
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want to participate in consensus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think consensus, or at least voting in it, is an optional aspect of this mode -- in the sense that we could also set the NODE_IS_VALIDATOR flag to false -- but I'll change the comment to reflect that.

@@ -28,6 +28,10 @@ Maintainer::start()
void
Maintainer::scheduleMaintenance()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like maintenance is fully controlled by Application, would be it cleaner to not initialize it there based on the mode, and assert in Maintainer's constructor that the mode is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. fixed.

std::shared_ptr<LedgerEntry const>
InMemoryLedgerTxnRoot::getNewestVersion(LedgerKey const& key) const
{
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment to explain that using the never-committing ledger txn implies that we must have the entire ledger state in memory (unless we disable ledger state loads for certain modes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some mention in ApplicationImpl.h but I'll expand it a bit.

src/main/Application.cpp Show resolved Hide resolved
// incorrectly.
static bool modeHasOverlay(AppMode m);
static bool modeHasDatabase(AppMode m);
static bool modePublishesHistory(AppMode m);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need modeWritesToBucketList? I'm happy to add it to my implementation (i disable the bucketlist anyway), but want to make sure we agree on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect we'll need a few more such mode judgements but I didn't have a clear enough idea of which ones we'd need. I'm going to leave this as-is for now since this PR isn't switching bucket behaviour (yet).

src/main/ApplicationImpl.h Outdated Show resolved Hide resolved
src/main/Application.h Show resolved Hide resolved
@@ -222,6 +229,11 @@ HistoryManagerImpl::getMaxLedgerQueuedToPublish()
bool
HistoryManagerImpl::maybeQueueHistoryCheckpoint()
{
if (!Application::modePublishesHistory(mApp.getMode()))
Copy link
Contributor

Choose a reason for hiding this comment

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

what about getMinLedgerQueuedToPublish/getMaxLedgerQueuedToPublish functions? They are in the public interface, and will break if called when DB is not configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed.

src/main/Application.cpp Outdated Show resolved Hide resolved
// Gets the current Application mode. This is a constant established
// when the Application is constructed and will not change while it
// runs.
virtual AppMode getMode() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of designing an interface that is easy to use correctly and hard to use incorrectly, I would prefer an interface with functions like "hasOverlay" over an interface with functions like "getMode". If we can remove this, that would be a good step toward avoiding errors in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, we have such interfaces as static helpers (Application::modeHasFoo(AppMode)); I figured it might be helpful to have the flexibility to switch or use the helpers in different contexts (and/or reify the mode as a value you might want to store / pass around). I guess these are hypotheticals though; can change if you feel strongly!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree, the code is littered with Application::modeHasDatabase(mApp.getMode()) .
Also, something like mApp.hasDatabase() is easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in all cases (though delegating to the static methods, which are still there for querying a mode before the application is built).

@@ -391,6 +391,50 @@ class AbstractLedgerTxnParent
// or if the corresponding LedgerEntry has been erased.
virtual std::shared_ptr<LedgerEntry const>
getNewestVersion(LedgerKey const& key) const = 0;

virtual uint64_t
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all be pure-virtual. Given the critical nature of LedgerTxn-type objects, we should make it as hard as possible for anyone implementing a subclass to forget to override functionality. For example, you didn't provide overrides for LedgerTxn even though the default implementations do not apply.

The reason these functions were originally excluded from the AbstractLedgerTxnParent interface is that they weren't needed for LedgerTxn. It should be possible to remove deleteObjectsModifiedOnOrAfterLedger entirely, and I don't think it would be too hard to implement LedgerTxn::countObjects. The drop* functions would probably be tricky to implement. Maybe we can find a way to avoid needing to implement any of these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reasonably comfortable changing them to "implemented as throw on anything other than a root LedgerTxn". Is that ok? They're never used outside of that context, and they seem pointlessly tricky to try to implement "for completeness" on other LedgerTxn subclasses.

// commits and answers null to all queries; then a "never-committing"
// sub-LedgerTxn is constructed beneath it that serves as the "effective"
// in-memory root transaction.
std::unique_ptr<InMemoryLedgerTxnRoot> mInMemoryLedgerTxnRoot;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use mLedgerTxnRoot? Looks like you could just change the type of that to std::unique_ptr<AbstractLedgerTxnParent>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Wow, I literally did not know unique_ptr would accept an abstract class as its type parameter. Learn something every day. Done!

@@ -888,12 +896,16 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData)
LedgerTxn ltxUpgrade(ltx);
Upgrades::applyTo(lupgrade, ltxUpgrade);

auto ledgerSeq = ltxUpgrade.loadHeader().current().ledgerSeq;
LedgerEntryChanges changes = ltxUpgrade.getChanges();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why even compute this unless there is a database?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment below.

static_cast<int>(i + 1));
if (Application::modeHasDatabase(mApp.getMode()))
{
auto ledgerSeq = ltxUpgrade.loadHeader().current().ledgerSeq;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right to me. You can't call ltxUpgrade.loadHeader() after calling ltxUpgrade.getChanges() because getChanges seals the LedgerTxn. What motivated you to change the way this was written in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Motivation shows up in later patch in the PR (00e8866b73abc478a6b3d78cdd09edbde161b551) where we need to have the changes pulled out early (and separately) in order to optionally send them to either or both of the stream and the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(That said, you're right it doesn't quite work as written, will fix)

@@ -970,6 +997,45 @@ LedgerManagerImpl::deleteOldEntries(Database& db, uint32_t ledgerSeq,
txscope.commit();
}

void
LedgerManagerImpl::setupLedgerCloseMetaStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow! I don't know how calling that got dropped from the diff, added back!

I guess I need to, uh, make some tests :)

@@ -67,11 +71,13 @@ class LedgerManagerImpl : public LedgerManager
CatchupConfiguration::Mode catchupMode);

void processFeesSeqNums(std::vector<TransactionFramePtr>& txs,
AbstractLedgerTxn& ltxOuter, int64_t baseFee);
AbstractLedgerTxn& ltxOuter, int64_t baseFee,
LedgerCloseMeta* ledgerCloseMeta);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why take the raw-pointer as argument instead of std::unique_ptr<LedgerCloseMeta> const&? Passing the smart-pointer makes ownership questions (eg. do I have to delete this?) much clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. Fixed.

@@ -512,6 +513,8 @@ TxSetFrame::getTotalFees(LedgerHeader const& lh) const
void
TxSetFrame::toXDR(TransactionSet& txSet)
{
// Should only call toXDR when in sorted-for-hash state.
releaseAssertOrThrow(mHashIsValid);
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 we can guarantee this currently: some places like StateSnapshot::writeHistoryBlocks, or HerderImpl::persistSCPState convert to XDR without necessarily calculating the hash (which sets mHashIsValid to true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm, ok. I'll call sortForHash explicitly. Sigh. This class is .. kinda a mess.

@graydon graydon force-pushed the bug-2226-fast-replay-streaming-txmeta branch 2 times, most recently from 5f4954e to 2fff91e Compare November 27, 2019 09:15
@graydon
Copy link
Contributor Author

graydon commented Nov 27, 2019

Rebased, addressed all review comments, added test.

@graydon graydon force-pushed the bug-2226-fast-replay-streaming-txmeta branch from 2fff91e to f44257d Compare November 28, 2019 00:30
@graydon
Copy link
Contributor Author

graydon commented Nov 28, 2019

Repushed with new test that runs replay mode and fixes a couple of the throwing methods on ledgertxn to call through to their parent so they can be used in that mode.

@@ -25,6 +25,7 @@ string ExternalQueue::kSQLCreateStatement =

ExternalQueue::ExternalQueue(Application& app) : mApp(app)
{
releaseAssertOrThrow(Application::modeHasDatabase(mApp.getMode()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Still need to remove checks for database existence from member functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -512,6 +513,7 @@ TxSetFrame::getTotalFees(LedgerHeader const& lh) const
void
TxSetFrame::toXDR(TransactionSet& txSet)
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to have this function as const (implied by its name and usage). can we explicitly call sortForHash() at the call sites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not .. entirely. Depends how much mess I make / clean up in the process. See comment below re: nicolas' question about the same thing.

@@ -333,6 +360,15 @@ ApplicationImpl::~ApplicationImpl()
{
mProcessManager->shutdown();
}
if (!modeHasDatabase(mAppMode))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because at least in this PR we're treating "no-database" mode as meaning "form buckets while you're running to check hashes, but don't keep buckets around when you shut down". This is debatable.

@@ -147,6 +148,12 @@ HistoryManagerImpl::logAndUpdatePublishStatus()
size_t
HistoryManagerImpl::publishQueueLength() const
{
if (!Application::modePublishesHistory(mApp.getMode()) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Only checking Application::modePublishesHistoryis sufficient here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies to getPublishQueueStates and publishQueuedHistory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to !modeHasDatabase() in all cases, removed modePublishesHistory()

@@ -147,6 +148,12 @@ HistoryManagerImpl::logAndUpdatePublishStatus()
size_t
HistoryManagerImpl::publishQueueLength() const
Copy link
Contributor

Choose a reason for hiding this comment

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

General question on this class: it looks like right now the choice between throwing and simulating function behavior is sort of arbitrary. For example, we allow to "publish" queued history, or to check publish queue length, but throw if we're trying to queue something up. It'd be nice to have consistent behavior across functions (note for the future: maybe at some point we can even not spin up HistoryManager at all when in "publishing disabled" mode, i think it's centrally controlled in LedgerManager anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately HistoryManager provides also the interface we use to do ledger-range calculations (based on whether or not we're in accelerated-time mode) and a few other query-history helpers, so there are several dozen call sites even in not-publishing mode. Again, I'm not strictly opposed to eliminating these / trying to move to a no-history-manager mode, but it involves yet more code motion and the PR is already pretty big.

@@ -147,6 +148,12 @@ HistoryManagerImpl::logAndUpdatePublishStatus()
size_t
HistoryManagerImpl::publishQueueLength() const
{
if (!Application::modePublishesHistory(mApp.getMode()) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need modePublishesHistory ? If you don't want to publish, just don't add a writeable archive in the first place... Less confusing than having arbitrary settings from the config getting ignored.

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 it happens we won't be able to publish history if we don't have a database -- the checkpoints all bounce off a database table that's not replicated in the no-database case, we just don't even store it.

So I could collapse this into modeHasDatabase, but we need to not-even-try to publish history when we're in a no-database mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

so maybe bomb out on startup if there is a writeable archive but we're configured with no database?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -512,6 +513,7 @@ TxSetFrame::getTotalFees(LedgerHeader const& lh) const
void
TxSetFrame::toXDR(TransactionSet& txSet)
{
sortForHash();
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to sort here? Is it out of paranoia? Call sites seem to call sortForHash already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because TxSetFrame and friends are a mess. AbstractTxSetFrame (which is all we have a pointer to in LedgerManagerImpl) does not have a sortForHash method, and shouldn't because SimulationTxSetFrame has only const members (this is a good design I want to keep).

In contrast, SimulationTxSetFrame::toXDR actually constructs, sorts-for-hash, and calls toXDR on a temporary TxSet, which it then throws out. I could theoretically do something like this in TxSetFrame::toXDR as well -- at least the construction-and-sorting part, the delegation has to bottom out with an xdrpp call somewhere! -- and remove or change the other call sites. Not sure what you prefer.

I should also point out that the not-very-well-named TxSetFrame::sortForApply doesn't actually sort-in-place either: it produces a new sorted vector for application. I'd be happy to try moving the sortForHash interface to this sort of shape too, but I wasn't sure how much deeper the pile of yak hair should get in this PR. Happy to go deeper if you like.

Basically TxSetFrame is a mutable/stateful type that's half way through migrating to an immutable/stateless type. I was trying to fit into that existing situation without knocking too many other things over.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was asking is: where in the code are you calling toXdr on a tx set that is not already in hash order? I can't find any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed to just assert that it's sorted

namespace stellar
{

LedgerCloseMetaStream::LedgerCloseMetaStream(asio::io_context& ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we gain anything from the extra level of buffering to write to the pipe?

For the "rebuild the meta" scenario, the "dumb" implementation would just synchronously write on the main thread (blocking). This would not require any throttling logic, would not do an extra copy of the data into buffers, and the only assumption would be that the reader would consume data as it gets pushed onto the pipe (and as we copy less, we may have a net perf increase).

If it's to make the "online" version work, it's probably premature as this design doesn't handle failures anyways (and this PR doesn't even deal with shutdown properly if not run with catchup).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


using namespace stellar;

#ifndef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that those tests are not written cross platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought I'd be using anonymous pipe() rather than a file-backed fd. I suppose the file-backed fd should work on windows too. I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

windows tests work now too. and confirmed named pipes still work.

clock.crank(true);
}
Hash hash = app->getLedgerManager().getLastClosedLedgerHeader().hash;
while (!app->getLedgerManager().metadataBufferEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not need to wait for the meta buffer to be empty (that seems to be missing logic in the app shutdown code: the "wrapper process" would not be able to do this) - regardless, I don't think we should support LIVE_NODE in this PR as there are too many unknowns around it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in process of making it synchronous.

@@ -1024,6 +1050,9 @@ handleCommandLine(int argc, char* const* argv)
runSignTransaction},
{"upgrade-db", "upgade database schema to current version",
runUpgradeDB},
{"replay-history-for-metadata",
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an option for the catchup sub-command instead of being exposed at the top level like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh. The comment I put in for modes specifically says it's for when cases where it does something meaningfully different than an existing subcommand. I think in this case it really does! catchup modifies local state; replay-history-for-metadata is stateless, idempotent. From the user's perspective it's quite different than catchup; it just happens to do catchup internally. No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, made into a catchup argument --replay-in-memory.

if (METADATA_OUTPUT_FILE_DESCRIPTOR != -1 &&
METADATA_OUTPUT_NAMED_PIPE != "")
{
LOG(FATAL) << "Can only set one of METADATA_OUTPUT_NAMED_PIPE and"
Copy link
Contributor

Choose a reason for hiding this comment

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

we could also only have one setting that can be either a path or an integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is not ideal: it's weird to the user (precludes integer-valued filenames, granted, unlikely) and requires weird implementation (a config-value visitor I guess? something that precludes accessing the string as a string when it's "really" an integer..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: went with "pathname which can be a pipe or a file, or 'fd:N' which opens the N file descriptor". User can deal with living without files named "fd:N".

@graydon
Copy link
Contributor Author

graydon commented Dec 5, 2019

Ok I believe this now addresses all review comments so far. Need to squash & rebase but it's (thankfully) getting quite small!

@graydon graydon force-pushed the bug-2226-fast-replay-streaming-txmeta branch from 8352181 to d107e08 Compare December 5, 2019 21:19
@graydon
Copy link
Contributor Author

graydon commented Dec 5, 2019

Squashed and formatted. Running tests now. Ought to be nearly ready.

@graydon graydon force-pushed the bug-2226-fast-replay-streaming-txmeta branch from d107e08 to b41b6cd Compare December 5, 2019 21:51
# existing open file descriptor N inherited by the process (for
# example to write to an anonymous pipe). Anonymous file descriptor
# streams are not supported on Windows (anonymous pipes do not work
# with the ASIO library used in stellar-core).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the asio comment is not accurate anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -512,6 +513,7 @@ TxSetFrame::getTotalFees(LedgerHeader const& lh) const
void
TxSetFrame::toXDR(TransactionSet& txSet)
{
sortForHash();
Copy link
Contributor

Choose a reason for hiding this comment

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

What I was asking is: where in the code are you calling toXdr on a tx set that is not already in hash order? I can't find any.

@@ -222,6 +230,11 @@ HistoryManagerImpl::getMaxLedgerQueuedToPublish()
bool
HistoryManagerImpl::maybeQueueHistoryCheckpoint()
{
if (!mApp.modeHasDatabase())
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already the check below that looks at hasAnyWritableHistoryArchive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -317,6 +333,11 @@ HistoryManagerImpl::publishQueuedHistory()
}
#endif

if (!mApp.modeHasDatabase())
Copy link
Contributor

Choose a reason for hiding this comment

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

meta question here...

I think we could simplify (and remove more of those if (!mApp.modeHasDatabase())) calls:
what if we were instead setting up an in memory sqlite database, that way code that deals with tables that are low churn like PersistentState would not have to be changed.
We could also not create the expensive tables at all, but we can also just check in tests that we don't populate them (and a bug in that front would be trivial to spot as we would have large memory usage).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible! But will defer this question to a later PR as discussed in chat.

// Note: BucketManager::dropAll will delete the $BUCKETDIR/tmp which is
// where process and history manager write _their_ temp files to, so we
// make sure to tear them down first here.
mProcessManager = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be done after joinAllThreads below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, good catch. sure, fixed.

@graydon graydon force-pushed the bug-2226-fast-replay-streaming-txmeta branch from b41b6cd to 6eff08a Compare December 6, 2019 19:21
@MonsieurNicolas
Copy link
Contributor

r+ 6eff08a

latobarita added a commit that referenced this pull request Dec 6, 2019
…txmeta

Bug 2226 fast replay streaming txmeta

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit 6eff08a into stellar:master Dec 6, 2019
@graydon graydon deleted the bug-2226-fast-replay-streaming-txmeta branch January 3, 2020 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants