chore: Filter in libwaku #3177
chore: Filter in libwaku #3177
Conversation
|
You can find the image built from this PR at Built from e7fcbdb5404bdb00ffc6c04c177ac2d4d71002f9 |
Ivansete-status
requested review from
NagyZoltanPeter,
gabrielmer,
richard-ramos and
darshankabariya
| @@ -331,10 +331,10 @@ proc setupProtocols( | |||
| except CatchableError: | |||
| return err("failed to mount waku lightpush protocol: " & getCurrentExceptionMsg()) | |||
|
|
|||
| mountLightPushClient(node) | |||
There was a problem hiding this comment.
Why is that?
I'm not satisfied with current client side mounting of lightpush and filter but to mount it all the time, maybe un-necessary. WDYT?
There was a problem hiding this comment.
GM @NagyZoltanPeter - by default we need to mount all the clients. We are also following that approach in store-clients.
| @@ -352,11 +352,11 @@ proc setupProtocols( | |||
| except CatchableError: | |||
| return err("failed to mount waku filter protocol: " & getCurrentExceptionMsg()) | |||
|
|
|||
| await node.mountFilterClient() | |||
There was a problem hiding this comment.
Yes indeed, good point!, and I agree that it may sound weird but it is handier from user PoV to always have the "client" components mounted so that is not needed an additional argument such as filter-client = true, or lightpush-client = true, or `store-client
There was a problem hiding this comment.
Well, imho, something more robust configuration option maybe needed. Like --light-protocols (edge-protocols?)
then install filter/lightpush/store-clients. Also we can leave the option to specify the service node(s) for the client protocols - but non mandatory. I think it can be a good balance between ease of use and control over resources.
I just noticed that with this change REST API will not function as expected. filter and lightpush REST endpoints are initialized upon --filternode and --lightpushnode having value.
Then this needs to be added here to keep consistency.
WDYT @Ivansete-status ?
There was a problem hiding this comment.
@NagyZoltanPeter - yes it makes 100% sense to me to add that --light-protocols option. That's something we will need shortly. I think we can consider that in a separate PR.
There was a problem hiding this comment.
Ok fine for me. I will approve this PR now!
| int waku_filter_unsubscribe_all(void* ctx, | ||
| WakuCallBack callback, | ||
| void* userData); | ||
|
|
There was a problem hiding this comment.
I think we need ping as well! Is it left out intentionally?
Without ping the client is forced to use subscribe all the time (in each 1-2 minutes) which is significantly higher load on service node than just check with ping.
There was a problem hiding this comment.
Thanks for that and very good point!
IMO, I think it is better to perform the ping internally and avoid the libwaku user handling that. I will submit another commit soon.
EDITED: I think it is better to refactor the current code base and put the filter-ping logic in one place.
There was a problem hiding this comment.
Do you mean, hide ping behind the scene? Let say ping every minute if any subscription is added and between unsubscribe_all?
There was a problem hiding this comment.
Do you mean, hide ping behind the scene? Let say ping every minute if any subscription is added and between unsubscribe_all?
Yes exactly, wdyt? Maybe we can suggest that in a separate PR
library/waku_thread/inter_thread_communication/requests/protocols/filter_request.nim
Outdated
Show resolved
Hide resolved
…ols/filter_request.nim Co-authored-by: gabrielmer <[email protected]>
No description provided.