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

feat: store API launch discovery process if needed #2092

Closed
wants to merge 1 commit into from

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Sep 28, 2023

Description

First step in a refactor of the way external APIs work. REST APIs requests now trigger a handler from the app layer. This new handler is responsible for tying peer manager, discv5 and node. It returns the result of the request without any knowledge of the internals of the REST APIs.

Next step would be to do the same for all APIs.

I'm still not sure of the timeout. Is it long enough? Should we loop wait until a peer is found?
Maybe start searching for a peer but return an error right away. Asking the client to try again later?

Changes

  • added a closure in app that ties discv5, peer manager and node
  • it's called by the STORE REST API
  • tests

Tracking #1941

@SionoiS SionoiS added the E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details label Sep 28, 2023
@SionoiS SionoiS self-assigned this Sep 28, 2023
@github-actions
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2092

Built from 77283f9

@SionoiS SionoiS changed the title feat: store API launch discovery process is needed feat: store API launch discovery process if needed Sep 28, 2023
@SionoiS SionoiS marked this pull request as ready for review September 29, 2023 12:30
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.

Tbh, I'm not sure about moving the actual logic of doing the query from the API. It seems to me that this is exactly where the querying logic should reside. What would make more sense to me is introducing something like a peerSelector function which gets passed down as an argument. WDYT?

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 2, 2023

Tbh, I'm not sure about moving the actual logic of doing the query from the API. It seems to me that this is exactly where the querying logic should reside. What would make more sense to me is introducing something like a peerSelector function which gets passed down as an argument. WDYT?

I disagree, I did it this way because the REST related code should not be mixed with what the request actually do, that way in the future adding some other APIs would be much easier. Another benefit would be that tests can be separated too.

API specific (request) -> app logic -> API specific (response)

@NagyZoltanPeter
Copy link
Contributor

@SionoiS I would have a generic comment here and it is a WDYT.
So, I have similar thought as @jm-clius about moving basically all logic away from the rest handler... given that as a rest handler I should trust a clojure given that it will do what I originally wanted.
On the other side I think it is tooo much knowledge outsourced to the app writer side.

I'm ok and happy with peer selection delegated outside. But maybe I would rather have a standard peer selection that could be overwritten by the app writer if there are other demands than basic functionality.

While moving away the knowledge of how to call the query is something is a bad idea.
As an app writer that something I would not like to know in that deep... at least me :-)
(The whole things warn me on an idiom of Non Virtual Interface - although it is a far analogue, but warns me it will lock ourself to keep this query interface or break the users code).

I'm thinking for a while and for me all these refactorings points to the direction that if we want to move functionalities from the node to app level, it would be better choice to introduce a mid layer, we can call it light_node/light_client.
This can free up app writer from a lot of overhead, could be maintained by us.

cc: @jm-clius, @Ivansete-status, @gabrielmer.

/Maybe it could be better discussed on discord or under an gh issue.../

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 4, 2023

@SionoiS I would have a generic comment here and it is a WDYT. So, I have similar thought as @jm-clius about moving basically all logic away from the rest handler... given that as a rest handler I should trust a clojure given that it will do what I originally wanted. On the other side I think it is tooo much knowledge outsourced to the app writer side.

I'm ok and happy with peer selection delegated outside. But maybe I would rather have a standard peer selection that could be overwritten by the app writer if there are other demands than basic functionality.

What if the closure was optional and defaulted to the logic in this PR?

While moving away the knowledge of how to call the query is something is a bad idea. As an app writer that something I would not like to know in that deep... at least me :-) (The whole things warn me on an idiom of Non Virtual Interface - although it is a far analogue, but warns me it will lock ourself to keep this query interface or break the users code).

Since the API calls have one purpose I don't see us changing the "interface" (closure's signature) often.

I'm thinking for a while and for me all these refactorings points to the direction that if we want to move functionalities from the node to app level, it would be better choice to introduce a mid layer, we can call it light_node/light_client. This can free up app writer from a lot of overhead, could be maintained by us.

I don't see what this mid layer would be responsible for. 🤔
IMO the app layer ties all the protocols & API together into something usable.
The node layer is only responsible for Waku specific protocols (all except discovery).

@NagyZoltanPeter
Copy link
Contributor

@SionoiS I would have a generic comment here and it is a WDYT. So, I have similar thought as @jm-clius about moving basically all logic away from the rest handler... given that as a rest handler I should trust a clojure given that it will do what I originally wanted. On the other side I think it is tooo much knowledge outsourced to the app writer side.
I'm ok and happy with peer selection delegated outside. But maybe I would rather have a standard peer selection that could be overwritten by the app writer if there are other demands than basic functionality.

What if the closure was optional and defaulted to the logic in this PR?

Exactly, we could have a default peer selection, that can be overridden by app writer person if needed.

While moving away the knowledge of how to call the query is something is a bad idea. As an app writer that something I would not like to know in that deep... at least me :-) (The whole things warn me on an idiom of Non Virtual Interface - although it is a far analogue, but warns me it will lock ourself to keep this query interface or break the users code).

Since the API calls have one purpose I don't see us changing the "interface" (closure's signature) often.

I don't mean often, but in this place we outsource the mean of the client call (REST API) to the app.
Is that better that the app knows how to perform a proper protocol client call on store?
I'm thinking about it as a more internal stuf and whenever we might need to change some checking's error handling anything to add between the call and http response transformation we are free to do so, if we control those interfaces.

On contrary as if I were the one using waku as a communication core of my application I would be happy to get such services ready made. In this term I get a rest interface I can use to communicate with my app without being involved too much in tiny details. Of course there is no barrier that can prevent use of node's client interface.

I'm thinking for a while and for me all these refactorings points to the direction that if we want to move functionalities from the node to app level, it would be better choice to introduce a mid layer, we can call it light_node/light_client. This can free up app writer from a lot of overhead, could be maintained by us.

I don't see what this mid layer would be responsible for. 🤔 IMO the app layer ties all the protocols & API together into something usable. The node layer is only responsible for Waku specific protocols (all except discovery).

Maybe I'm totally wrong with that design aim. For me all these layers are service layers. Node, API, ...
Recently there are plenty of discussions around the bounties, from those I filtered out that we somehow must distinguish between a full node (maybe call as network node - that performs the waku network) and light client that is more a core of an user application (chat program, etc.). In my idealistic idea such a light_node could help app writers more with its slim interface that is more obvious to use than the full node interface.
Of course maybe it is not the place and time for it, the reason it came to my mind is that it can be a place to delegate such handlers there rather than to the application itself.
Clojures are good, but in this term maybe not proper for abstraction. A bit more conventional OOP abstraction could help more. That's my feelings, nothing strict in it.

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 4, 2023

I don't mean often, but in this place we outsource the mean of the client call (REST API) to the app. Is that better that the app knows how to perform a proper protocol client call on store? I'm thinking about it as a more internal stuf and whenever we might need to change some checking's error handling anything to add between the call and http response transformation we are free to do so, if we control those interfaces.

On contrary as if I were the one using waku as a communication core of my application I would be happy to get such services ready made. In this term I get a rest interface I can use to communicate with my app without being involved too much in tiny details. Of course there is no barrier that can prevent use of node's client interface.

An app that does not use discovery would not be able to use the APIs if discovery is internal to them.
My solution (passing a handler with a default) do have ready-made logic but allow custom handlers.

Maybe I'm totally wrong with that design aim. For me all these layers are service layers. Node, API, ... Recently there are plenty of discussions around the bounties, from those I filtered out that we somehow must distinguish between a full node (maybe call as network node - that performs the waku network) and light client that is more a core of an user application (chat program, etc.). In my idealistic idea such a light_node could help app writers more with its slim interface that is more obvious to use than the full node interface. Of course maybe it is not the place and time for it, the reason it came to my mind is that it can be a place to delegate such handlers there rather than to the application itself. Clojures are good, but in this term maybe not proper for abstraction. A bit more conventional OOP abstraction could help more. That's my feelings, nothing strict in it.

Full vs Light node is tricky because it depends on which protocol the app uses.
Is an app with only relay a light node? What about a node with; store, filter, light push and peer exchange is that a light node?
Building for modularity is hard...

In Nim our choice of abstraction seams limited (only by my knowledge of Nim maybe).

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 4, 2023

@jm-clius @NagyZoltanPeter Check #2109 as an alternative

@SionoiS SionoiS closed this Oct 4, 2023
@SionoiS SionoiS deleted the feat--store-api-discovery branch January 10, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants