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

refactor(cmd-api-server): add socket.io connection to each ledger-connector #1602

Closed
takeutak opened this issue Nov 30, 2021 · 8 comments · Fixed by #1660
Closed

refactor(cmd-api-server): add socket.io connection to each ledger-connector #1602

takeutak opened this issue Nov 30, 2021 · 8 comments · Fixed by #1660
Assignees
Labels
enhancement New feature or request

Comments

@takeutak
Copy link
Member

takeutak commented Nov 30, 2021

Is your feature request related to a problem? Please describe.
On the current codes, cmd-api-server does not have socket.io connection to each ledger-connector. This communication channel is needed for developing the APIs of sendSyncRequest, sendAsyncRequest, and startMonitor as these are developed in cmd-socket-server via socket.io connection to each ledger.
This feature was already developed in cmd-socket-server, but in case of merging those codes as #1502, we need to develop it.

Describe the solution you'd like
Add the socket.io connection to cmd-api-server

EPIC
#1605

@petermetz
Copy link
Member

@takeutak Could you please expand on this part? On the current codes, cmd-api-server does not have socket.io connection to each ledger-connector. Support for socket.io was added a long time ago.

@takeutak
Copy link
Member Author

takeutak commented Dec 6, 2021

@petermetz Thanks for your advice!
As I received your advice, I looked the codes again.

Let me clarify one thing, is the following figure right for calling your mentioned codes?

According to the figure, BLP sends requests to API clients on each ledger connector via socket.io connection (*1), and API clients sends requests to ledger connector via socket.io connection (*2), is it right?
(*1) BLP <--(socket.io)--> API clients on each ledger connector
(*2) API clients on each ledger connector <--(socket.io)--> ledger connector

I think your implementation is good, but it makes more useful to prepare the following ways:

  • abstraction of calling API client on each ledgers when (*1) is executed. I think Verifier structure resolve the feature.
  • extension of socket.io connection structure to other ledgers than besu (Currently, socket.io connections seem to be developed only on besu connector in your implementation)

To be clear, this issue is not intended to remove the features you already developed. It is only intended to enhance the features.

@petermetz petermetz assigned takeutak and izuru0 and unassigned takeutak Dec 6, 2021
@outSH
Copy link
Contributor

outSH commented Dec 10, 2021

@petermetz Please review this implementation idea and share your comments (especially on preferred location of new components - SocketIOApiClient and Verifier)

The general idea is to divide the current Verifier from cmd-socketio into two components that will be more compatible with api-server architecture.

SocketIOApiClient

  • Will abstract the communication with any socketio connector (validator).
  • Will use common interface with currently existing BesuApiClient (socketio part), and any future OpenAPI connectors with socketio support.
  • In opposite to BesuApiClient, SocketIOApiClient will work with any socketio validator (but not with openapi-socketio ones - like besu, this can be a little misleading but I don't see a way around it at the moment).
  • Most of the implementation will be taken from Verifier without much change in implementation (I'd prefer making future refactors in separate PR for better change tracking, even if they seem urgent).
  • The only major change in SocketIOApiClient will be watchBlocksV1 function that will return Observable (to match BesuApiClient).
  • Not sure where to place it yet. I didn't find a place where it would fit perfectly, and adding new package seems a little too much for so little logic in this component. I'm thinking about packages/cactus-api-client/src/main/typescript/socketio-api-client.ts (next to Verifier).

Verifier (ledger monitor proxy)

  • Will take compatible ledger ApiClient as an ctor argument (at the moment only SocketIOApiClient and BesuApiClient).
  • I want to put it in packages/cactus-api-client/src/main/typescript/Verifier.ts (similar intent as described below)
  • Just like current socketio Verifier it will:
    • Reuse conenctions to same ledger connector hosts.
    • Keep track of monitoring process.
    • Expose callback based startMonitor/stopMonitor
  • In the future it could be integrated into ApiClient (packages/cactus-api-client/src/main/typescript/api-client.ts), because they seems to have a similar goal - "providing additional functionality to the DefaultApi". The problem - ApiClient should work both on the front and backend (already used in Angular app in one example), so there would be some work needed to make socketio communication platform independent. Also - more API discussion is needed, so this is topic for another issue/PR.

While implementing this I'll try to be api-server native (keeping #1502 in mind). I'll try to use this common code implementation in cmd-socketio to prevent code duplication, but it will be lower priority for me (if that's OK).

Here is the UML draft for new Verifier:
verifier uml

@petermetz
Copy link
Member

petermetz commented Dec 13, 2021

@outSH Thank you for the details. How will the ISocketApi deliver the mentioned abstraction to be ledger-agnostic when it comes to the WatchBlocksV1Progress type? E.g., what will that type look like that somehow works with all ledgers that we added/will add in the future to the list of supported ones?

@outSH
Copy link
Contributor

outSH commented Dec 14, 2021

@petermetz I don't have a definite plan for that yet, I'll experiment and see what fits the best. I was thinking about replacing WatchBlocksV1Progress to unknown in the interface (since Verifeir doesn't really care about the block structure), but for more type safety I'm now thinking about making ISocketApi generic on block type. Verifier'd then infer (with conditional type) the block type and use it in it's declarations (so passing BesuAPI would require onEvent() callback accepting only besu block class) - but it's just a rough idea at the moment, will see how it works in practice when I implement it.

Do you have any other comments regarding this feature?

@petermetz
Copy link
Member

@outSH

I don't have a definite plan for that yet, I'll experiment and see what fits the best.

More details would be very welcome as they come along. The reason I'm highly interested in the details of this specifically because the "common abstraction" seems to be the foundational premise for everything else that's in the issue.


I want to put it in packages/cactus-api-client/src/main/typescript/Verifier.ts (similar intent as described below)

Any chance that the filename conventions in place could be kept? (lower-hypen-case)


In opposite to BesuApiClient, SocketIOApiClient will work with any socketio validator (but not with openapi-socketio ones - like besu, this can be a little misleading but I don't see a way around it at the moment).

Calling it SocketIOApiClient would imply that there hasn't been SocketIO support added already a long time ago to the other one. Is there something else that refers to a real distinction that we could put in the name of it?


Is the Verifier#eventListenerHash somehow replicated or does this store state locally that breaks implicit horizontal scalability/auto-scaling in a production environment?

outSH added a commit to outSH/cactus that referenced this issue Dec 15, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
@outSH
Copy link
Contributor

outSH commented Dec 15, 2021

@petermetz @takeutak @izuru0

I've finished an initial implementation, published as draft PR here -> #1660. Please review and share your comments. There are some more things to do, but the main logic is already there.

@petermetz

More details would be very welcome as they come along. The reason I'm highly interested in the details of this specifically because the "common abstraction" seems to be the foundational premise for everything else that's in the issue.

In the end ISocketApi (renamed to ISocketApiClient) uses generic type to work with any ledger block/event data format (see how besu-api-client implements ISocketApiClient). This can be extended to use generics for other specific parts of the interface like in IPluginLedgerConnector. I'm also thinking about splitting this interface into smaller interfaces (suitable for mixin).

Any chance that the filename conventions in place could be kept? (lower-hypen-case)

Yes, of course, it was a copy-paste typo.

Calling it SocketIOApiClient would imply that there hasn't been SocketIO support added already a long time ago to the other one. Is there something else that refers to a real distinction that we could put in the name of it?

Yes, I'm not satisfied with the name either, ( but unfortunately couldn't think of anything more suitable, I'm very open to suggestions :) It's meant to work with current, non-openapi based socketio interfaces, maybe we could add Generic, Common or Legacy prefix to it, or something else, but I'm afraid the name would be too mouthful. Maybe ValidatorApiClient?

Is the Verifier#eventListenerHash somehow replicated or does this store state locally that breaks implicit horizontal scalability/auto-scaling in a production environment?

I made a change and Verifier has a one-to-one relation to the ApiClient now. eventListenerHash has been replaced with runningMonitors, which are just named subscriptions to watchBlock subject.

outSH added a commit to outSH/cactus that referenced this issue Dec 16, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 16, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
@petermetz
Copy link
Member

@outSH Got it, thank you for the adjustments so far. I'll head over to the draft PR and we can have a much more targeted discussion over there.

Yes, I'm not satisfied with the name either, ( but unfortunately couldn't think of anything more suitable, I'm very open to suggestions :) It's meant to work with current, non-openapi based socketio interfaces, maybe we could add Generic, Common or Legacy prefix to it, or something else, but I'm afraid the name would be too mouthful. Maybe ValidatorApiClient?

Based on my hope that we'll end up consolidating I would go with Legacy, that way it might be less confusing to people that there are still somehow to implementations of something that are seemingly doing the same thing.

I made a change and Verifier has a one-to-one relation to the ApiClient now. eventListenerHash has been replaced with runningMonitors, which are just named subscriptions to watchBlock subject.

Awesome, thank you for updating it like that!

outSH added a commit to outSH/cactus that referenced this issue Dec 17, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 20, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 20, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 21, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 29, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 29, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this issue Dec 29, 2021
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
petermetz pushed a commit that referenced this issue Jan 4, 2022
… api-server and api-client

Full description of changes and planning log available in #1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: #1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
aldousalvarez pushed a commit to aldousalvarez/cactus that referenced this issue Jan 19, 2022
… api-server and api-client

Full description of changes and planning log available in hyperledger#1602. In
general, this commit adds two new components - one for communicating
with plain socketio validators using socketio interface, and verifier
that implements similar features as socketio-server verifier.

Closes: hyperledger#1602
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
Signed-off-by: aldousalvarez <aldousss.alvarez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants