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

Jump to bottom

misc(syncer): make timeout configurable #155

Closed
vgonkivs opened this issue Feb 1, 2024 · 6 comments
Closed

misc(syncer): make timeout configurable #155

vgonkivs opened this issue Feb 1, 2024 · 6 comments
Labels
good first issue Good for newcomers syncer

Comments

@vgonkivs
Copy link
Member

vgonkivs commented Feb 1, 2024 •
edited
Loading

We should add additional field to the syncer config struct - maxAwaitingTime that specifies max time for processing Head request in case we are getting it from the network. This field should be configurable on the startup.
@vgonkivs vgonkivs added syncer good first issue Good for newcomers labels Feb 1, 2024
@AryanGodara
Copy link
Contributor

Can I work on this issue @vgonkivs?
Also, in case a value isn't provided, do we take 2s as the default value?

@vgonkivs
Copy link
Member Author

vgonkivs commented Mar 3, 2024

Hey, @AryanGodara. Yes, sure you can take it. Thank you 🙂. Yes, the default value for maxAwaitingTime should be 2 seconds.

@AryanGodara
Copy link
Contributor

Hey, @AryanGodara. Yes, sure you can take it. Thank you 🙂. Yes, the default value for maxAwaitingTime should be 2 seconds.

Thanks @vgonkivs :D I'll be pushing a PR for this shortly!

@AryanGodara AryanGodara mentioned this issue Mar 3, 2024
Closed
5 tasks
@renaynay
Copy link
Member

renaynay commented Mar 7, 2024

Making a comment here, and this is a broader comment about configuration options in celestia-node / go-header:

Not everything deserves to be a configurable parameter. IMO, it's not worth the additional code to allow something as granular as a context timeout on a head request inside the syncer to be configurable. We should set a good constant and keep to it, and if it proves ineffective, then we bump it or try to figure out why requests are taking so long.

We should be a bit more picky about what deserves configurability vs what we can just hardcode. It is unlikely anyone external to the development team will try to configure this param.

@renaynay renaynay mentioned this issue Mar 7, 2024
Merged
@renaynay
Copy link
Member

renaynay commented Mar 7, 2024

Closing as unplanned

@renaynay renaynay closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2024
@Wondertan
Copy link
Member

Agreed. @AryanGodara, I suggest looking at #149 instead

Wondertan pushed a commit that referenced this issue Mar 14, 2024
@renaynay
chore(sync)!: Rename unrecentHead to outdatedHead in metrics + cl…
b9fa9aa 
…ean up `Head` func a bit (#163)

This PR renames a poorly named metric from `unrecent_header` to `outdated_head` which is more descriptive / accurate. 

It also cleans up a bit of the code inside of `sync_head.go`.

This PR is meant to override #162 if we agree that the timeout should not be configurable (see [comment](#155 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers syncer
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

misc(syncer): make timeout configurable AryanGodara/go-header
4 participants
@Wondertan @vgonkivs @renaynay @AryanGodara