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

Add basic retry functionality #1119

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
+650 −396
Open

Add basic retry functionality #1119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3f030e3
adding basic retry functionality
dryajov Feb 18, 2025
25c0c16
avoid duplicate requests and batch them
dryajov Feb 18, 2025
62dc92c
fix cancelling blocks
dryajov Feb 18, 2025
94926ad
properly resolve blocks
dryajov Feb 18, 2025
d399654
minor cleanup - use `self`
dryajov Feb 18, 2025
cce0b17
avoid useless asyncSpawn
dryajov Feb 18, 2025
5507e88
track retries
dryajov Feb 18, 2025
6b4e587
limit max inflight and set libp2p maxIncomingStreams
dryajov Feb 18, 2025
2a44d5d
cleanup
dryajov Feb 18, 2025
255edf8
add basic yield in readLoop
dryajov Feb 18, 2025
4cf466e
use tuple instead of object
dryajov Feb 18, 2025
5611cf4
cleanup imports and logs
dryajov Feb 18, 2025
ad8340a
increase defaults
dryajov Feb 18, 2025
2671587
wip
dryajov Feb 18, 2025
f455022
fix prefetch batching
dryajov Feb 18, 2025
b090162
cleanup
dryajov Feb 18, 2025
f7dc789
decrease timeouts to speedup tests
dryajov Feb 18, 2025
a806be0
remove outdated test
dryajov Feb 18, 2025
616672d
add retry tests
dryajov Feb 18, 2025
ac27292
should track retries
dryajov Feb 18, 2025
7701e75
remove useless test
dryajov Feb 18, 2025
732de6c
use correct block address (index was off by 1)
dryajov Feb 19, 2025
45af78a
remove duplicate noop proc
dryajov Feb 19, 2025
aeb3ac6
add BlockHandle type
dryajov Feb 19, 2025
83bd438
Use BlockHandle type
dryajov Feb 19, 2025
b619482
add fetchLocal to control batching from local store
dryajov Feb 19, 2025
1c98b31
add format target
dryajov Feb 19, 2025
999ed6e
revert deps
dryajov Feb 19, 2025
2e97fe8
adjust quotaMaxBytes
dryajov Feb 19, 2025
18b5d6c
Merge branch 'master' into feat/add-retry-functionality
dryajov Feb 20, 2025
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,5 @@ docker/prometheus-data
.DS_Store
nim.cfg
tests/integration/logs

data/
5 changes: 5 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ nph/%: build-nph
echo -e $(FORMAT_MSG) "nph/$*" && \
$(NPH) $*

format:
$(NPH) *.nim
$(NPH) codex/
$(NPH) tests/

Copy link
Member

Choose a reason for hiding this comment

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

ehehe good easter egg... 😃

clean-nph:
rm -f $(NPH)

Expand Down
2 changes: 0 additions & 2 deletions codex/blockexchange/engine/discovery.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,6 @@ proc start*(b: DiscoveryEngine) {.async.} =
asyncSpawn fut

b.discoveryLoop = b.discoveryQueueLoop()
b.trackedFutures.track(b.discoveryLoop)
asyncSpawn b.discoveryLoop
Copy link
Member

Choose a reason for hiding this comment

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

Hm... why are you excluding this? This will cause the discovery loop not to stop when someone calls .stop() on the discovery service. Or rather, it will stop cause Codex will exit anyway, but won't receive a cancel signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not really - this doesn't affect stopping since we call cancel the tracked futures on stop. The asyncSpawn call tries to do some logging and re-throw any future error as a Defect, but that was really a workaround because we lacked checked exceptions on async code. I do intend to make tracked futures only accept Future.Raises([]) for that matter.

Copy link
Member

@gmega gmega Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Wait but you're not adding the discoveryLoop future to the trackedFutures, so that one won't stop.


proc stop*(b: DiscoveryEngine) {.async.} =
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are changing b => self all over the places, maybe you want to do it here as well?

## Stop the discovery engine
Expand Down
Loading
Loading