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

misc(syncer): make timeout configurable #162

Closed

Conversation

AryanGodara
Copy link
Contributor

@AryanGodara AryanGodara commented Mar 3, 2024

Overview

Resolves #155

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@AryanGodara
Copy link
Contributor Author

@vgonkivs This one's ready for review :)

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

This is wrong. The State struct is not supplied to the Syncer, but the Syncer provides it's cuttent state to user through State method on Syncer. Therefore, you can't configure anything through it in Syncer.

Instead, we should add this configuration field to the Config

@AryanGodara
Copy link
Contributor Author

This is wrong. The State struct is not supplied to the Syncer, but the Syncer provides it's cuttent state to user through State method on Syncer. Therefore, you can't configure anything through it in Syncer.

Instead, we should add this configuration field to the Config

I see. I'm sorry for this error, I'll make the changes asap!

@vgonkivs
Copy link
Member

vgonkivs commented Mar 6, 2024

hey @AryanGodara, thanks for you contribution. Let me help you to improve your code: If you take a look here, you will see the Parameter struct that allows configuring the syncer. In the syncer c-tor we create default parameters and override them using options pattern, which you can find in the same files I've added previously. The idea is to add maxAwait to the Parameter struct + set the default value in DefaultParameters and add the ability to override the default maxAwait time.

@AryanGodara
Copy link
Contributor Author

hey @AryanGodara, thanks for you contribution. Let me help you to improve your code: If you take a look here, you will see the Parameter struct that allows configuring the syncer. In the syncer c-tor we create default parameters and override them using options pattern, which you can find in the same files I've added previously. The idea is to add maxAwait to the Parameter struct + set the default value in DefaultParameters and add the ability to override the default maxAwait time.

Wow thank you for explaining it in such detail @vgonkivs :D
This was extremely helpful. I've just pushed new changes. I hope this time, I've implemented it correctly (:

@renaynay
Copy link
Member

renaynay commented Mar 7, 2024

Hi @AryanGodara thank you so much for your contribution. We've decided that we don't actually need this feature unfortunately. I apologise for the last minute change of mind. There are many other good first issues that we welcome you to work on, however!

@renaynay renaynay closed this Mar 7, 2024
@AryanGodara
Copy link
Contributor Author

Hi @AryanGodara thank you so much for your contribution. We've decided that we don't actually need this feature unfortunately. I apologise for the last minute change of mind. There are many other good first issues that we welcome you to work on, however!

It's quite alright @renaynay :D
It was a good issue to get onboarded to the project. I hope I'll continue making new good contributions over time :D

Wondertan pushed a commit that referenced this pull request Mar 14, 2024
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

misc(syncer): make timeout configurable
4 participants