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

Easier to capture scp messages #3832

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Conversation

JuI3s
Copy link
Contributor

@JuI3s JuI3s commented Jul 17, 2023

Description

Draft working on #3769

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@JuI3s JuI3s marked this pull request as draft July 17, 2023 09:17
@JuI3s JuI3s changed the title Capture scp messages Easier to capture scp messages Jul 17, 2023
@JuI3s JuI3s marked this pull request as ready for review July 17, 2023 12:03
@JuI3s JuI3s force-pushed the capture-scp-messages branch 2 times, most recently from 8680e45 to dd73764 Compare July 17, 2023 12:39
@mbsdf
Copy link

mbsdf commented Jul 18, 2023

local:2/4/100%/51.0s parallel: This job failed:
runpart 1 'application setup' 'XDRSHA256 is identical to byte SHA256' 'bucketmanager do not leak empty-merge futures' 'History publish to multiple archives' 'parse peer record'
FAIL: test/selftest-pg

Copy link
Contributor

@MonsieurNicolas MonsieurNicolas left a comment

Choose a reason for hiding this comment

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

general approach probably has too many implications (which would require a lot of new tests to be written).
Also: rebase branches instead of merging as this creates hard-to-follow git history

src/herder/HerderImpl.cpp Outdated Show resolved Hide resolved
src/herder/HerderImpl.cpp Outdated Show resolved Hide resolved
@JuI3s JuI3s force-pushed the capture-scp-messages branch 3 times, most recently from 7462440 to baf5229 Compare July 19, 2023 06:46
src/ledger/LedgerManagerImpl.cpp Outdated Show resolved Hide resolved
src/ledger/LedgerManagerImpl.h Outdated Show resolved Hide resolved
src/herder/Herder.h Outdated Show resolved Hide resolved
@JuI3s JuI3s requested a review from marta-lokhova July 20, 2023 08:58
@JuI3s JuI3s force-pushed the capture-scp-messages branch 11 times, most recently from 0245389 to 2c93596 Compare July 25, 2023 02:28
src/herder/Herder.cpp Outdated Show resolved Hide resolved
src/ledger/LedgerManagerImpl.h Outdated Show resolved Hide resolved
src/history/HistoryManagerImpl.cpp Outdated Show resolved Hide resolved
src/history/HistoryManagerImpl.cpp Outdated Show resolved Hide resolved
@JuI3s JuI3s requested a review from marta-lokhova July 27, 2023 01:44
Copy link
Contributor

@marta-lokhova marta-lokhova left a comment

Choose a reason for hiding this comment

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

Couple of minor suggestions, otherwise LGTM

@@ -142,6 +142,7 @@ Config::Config() : NODE_SEED(SecretKey::random())
EXPERIMENTAL_BUCKETLIST_DB_INDEX_PAGE_SIZE_EXPONENT = 14; // 2^14 == 16 kb
EXPERIMENTAL_BUCKETLIST_DB_INDEX_CUTOFF = 20; // 20 mb
EXPERIMENTAL_BUCKETLIST_DB_PERSIST_INDEX = true;
PUBLISH_TO_ARCHIVE_DELAY = std::chrono::seconds{2};
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, should we set this to 0 by default, and operators that need this functionality can adjust the config manually?


auto time = app.getClock().now();
auto dur = time - start;
return std::chrono::duration_cast<std::chrono::seconds>(dur) >
Copy link
Contributor

Choose a reason for hiding this comment

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

>=? This way you don't need to special case std::chrono::seconds::zero

capture scp messages draft

remove extra white space

clean up header impl

moved delay publish logic to ledger manager

rm delay timer from herder impl and add delay-publish-to-archive criterion

add delay publish to archive config option

preliminary changes for delay publish to archive logic in history manager impl

Remove delay publish to archive logic from herder and ledger manager.

Change delay-publish-to-archive work to ConditionalWork.

update xdr

make publish to archive delay configurable

fix format

fix format and work naming

Make default publish-to-archive delay to 0
@marta-lokhova
Copy link
Contributor

r+ 419329c

@latobarita latobarita merged commit debd190 into stellar:master Aug 11, 2023
8 checks passed
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