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

Restructure block listener retry configuration #158

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

Chengxuan
Copy link
Contributor

@Chengxuan Chengxuan commented Dec 5, 2024

Move the block listener retry configuration outside the retry attribute as that was for resty client retry. Having them together is confusing.

Signed-off-by: Chengxuan Xing <[email protected]>
@Chengxuan Chengxuan marked this pull request as ready for review December 6, 2024 09:37
@Chengxuan Chengxuan requested a review from a team as a code owner December 6, 2024 09:37
Signed-off-by: Chengxuan Xing <[email protected]>
@Chengxuan Chengxuan requested a review from EnriqueL8 December 9, 2024 14:07
@Chengxuan
Copy link
Contributor Author

@EnriqueL8 Migration logic added, ready for another look

Signed-off-by: Chengxuan Xing <[email protected]>
@EnriqueL8 EnriqueL8 changed the title restructure config Restructure block listener retry configuration Dec 10, 2024
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

After looking the PR - I think bigger changes are needed here. So we use these delay and retry configurations across three things:

  • Block Listener
  • Event Listener
  • Event Stream

If we are deciding to split one, we should split them all to be configurable. Right not this PR proposes only splitting block listeners but those configurations still apply to the other two which is odd

@Chengxuan
Copy link
Contributor Author

Chengxuan commented Dec 11, 2024

@EnriqueL8 thanks for the question. Even though the retry object appeared in the 3 different concepts you listed above, the loop it controls all serves the same purpose: to resume the listener processes that all require making further JSON-RPC queries. Because normally when one of them hits an issue, the others will likely suffer the same issue, having a consistent retry setting makes sense. So a single configuration for all makes sense and avoids giving users too many toggles before it's necessary.

I'll pick a better name for the configuration to make that clear.

@Chengxuan Chengxuan requested a review from EnriqueL8 December 11, 2024 06:37
@Chengxuan
Copy link
Contributor Author

@EnriqueL8 ready for another look again

Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Makes sense now! Thanks for the clarification @Chengxuan

@EnriqueL8 EnriqueL8 merged commit c80b0f3 into hyperledger:main Dec 11, 2024
4 checks passed
@EnriqueL8 EnriqueL8 deleted the refactor-retry-config branch December 11, 2024 10:34
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.

2 participants