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

fix: waku_filter_v2/common: PEER_DIAL_FAILURE ret code change: 200 -> 504 #2236

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Changing the return code to a more meaningful value when PEER_DIAL_FAILURE happens.

@Ivansete-status Ivansete-status marked this pull request as ready for review November 22, 2023 09:41
Copy link

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2236

Built from 350e384

@Ivansete-status Ivansete-status self-assigned this Nov 22, 2023
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM and this should definitely be an error code.

Note however that it was chosen to correspond with what is already implemented for Store:

PEER_DIAL_FAILURE = uint32(200)
and would have to be fixed there as well.

Less important, but something to consider:
504 is also the HTTP error code for Gateway Timeout, which is very similar/equivalent to peer dial failure. Should we consider sticking to HTTP terminology? All other error codes are chosen to mirror the HTTP codes

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Much better thanks!

@@ -13,11 +13,11 @@ const
type
FilterSubscribeErrorKind* {.pure.} = enum
UNKNOWN = uint32(000)
PEER_DIAL_FAILURE = uint32(200) # TODO shouldn't this be an error code, e.g. 504 Gateway Timeout?
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a couples of TODOs in the code base and I always think to myself. Why not just change it instead of adding a TODO your touching this part of the code anyway. If you need to remember to do something later should open an issue IMO.

@Ivansete-status
Copy link
Collaborator Author

Less important, but something to consider: 504 is also the HTTP error code for Gateway Timeout, which is very similar/equivalent to peer dial failure. Should we consider sticking to HTTP terminology? All other error codes are chosen to mirror the HTTP codes

Thanks for the heads-up!
The idea is to follow the HTTP terminology.

@Ivansete-status Ivansete-status merged commit 6301bec into master Nov 30, 2023
9 of 10 checks passed
@Ivansete-status Ivansete-status deleted the fix-error-code-filter-v2 branch November 30, 2023 09:47
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.

4 participants