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

feat: API discovery handler #2109

Merged
merged 2 commits into from
Oct 27, 2023
Merged

feat: API discovery handler #2109

merged 2 commits into from
Oct 27, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Oct 4, 2023 •
edited
Loading

Description

REST APIs now take an optional handler that is responsible for discovery of peer if needed.

Changes

  • Added default discovery handler
  • Store, Filter, LightPush now have an optional discovery handler.

Tracking #1941

@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2109

Built from 5bc1729

This was referenced Oct 4, 2023
Closed
Closed
@SionoiS SionoiS changed the title feat: store API discovery handler feat: API discovery handler Oct 4, 2023
@SionoiS SionoiS marked this pull request as ready for review October 4, 2023 20:21
NagyZoltanPeter
NagyZoltanPeter requested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

In general I like the way you do! Thank you for taking previous criticism such a positive way.
I have some questions transformed into comments.
In filter postput handler I think we miss the handler call... hence the request for change. Otherwise happy to approve.

waku/waku_api/rest/filter/handlers.nim Show resolved Hide resolved
waku/waku_api/rest/filter/handlers.nim Show resolved Hide resolved
waku/waku_api/rest/store/handlers.nim Show resolved Hide resolved
Ivansete-status
Ivansete-status requested changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

In general it looks great!
However, I wonder if we could adapt it a little bit.
I've added a few comments that I hope are useful.

waku/waku_api/rest/filter/handlers.nim Outdated Show resolved Hide resolved
apps/wakunode2/app.nim Show resolved Hide resolved
apps/wakunode2/app.nim Show resolved Hide resolved
waku/waku_api/rest/filter/handlers.nim Show resolved Hide resolved
@SionoiS SionoiS added the E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details label Oct 5, 2023
NagyZoltanPeter
NagyZoltanPeter approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Thank you! I think all these a great step forward for a cleaner architecture.
... meanwhile thinking of / whishing for IoC/DI style that is quite applicable in such cases.
(if you happen to know Java reflection and how spring boot can utilize - but even I studied such solutions for c++, also possible even though not that slim and nice).

waku/waku_api/handlers.nim Show resolved Hide resolved
@SionoiS SionoiS requested a review from jm-clius October 24, 2023 19:05
Ivansete-status
Ivansete-status approved these changes Oct 27, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

After a chat with @SionoiS, I now understand better the purpose of the PR and it looks good to me.
Thanks!

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 27, 2023

I will make the changes suggested by @Ivansete-status and then we gucci.

@SionoiS SionoiS force-pushed the feat--store-discovery-handler branch from 743e78c to 1b77aeb Compare October 27, 2023 12:34
@SionoiS
discovery handler
7e5d40b 
added defaults

other apis

Added timeout & missing call to disco

Error Msgs
@SionoiS SionoiS force-pushed the feat--store-discovery-handler branch from 6d03caf to 7e5d40b Compare October 27, 2023 14:48
@SionoiS SionoiS merged commit 7ca516a into master Oct 27, 2023
9 of 10 checks passed
@SionoiS SionoiS deleted the feat--store-discovery-handler branch October 27, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@NagyZoltanPeter NagyZoltanPeter NagyZoltanPeter approved these changes

@jm-clius jm-clius Awaiting requested review from jm-clius

Assignees

@SionoiS SionoiS

Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SionoiS @NagyZoltanPeter @Ivansete-status