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

Add Deployment documentation to architecture snapshot #121

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

kamirr
Copy link
Contributor

@kamirr kamirr commented Nov 4, 2024

This change is Reviewable

@kamirr kamirr requested a review from dopiera November 4, 2024 12:39
Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dopiera and @kamirr)


arch-snapshot/arch.md line 2389 at r1 (raw file):

  * Identity
  * Networking
  * GSB

I think we don't treat GSB as separate component but a way to communicate between components


arch-snapshot/arch.md line 2393 at r1 (raw file):

  * Payments
    * Payment Drivers
* Requestor Agent is a separate process, usually implemented using an SDK

Do we describe anywhere how do you build an requestor, what is it and what are the interactions?
If yes than please link it, if not it should be described


arch-snapshot/arch.md line 2418 at r1 (raw file):

    - QEMU if Activity is deployed and using VM Runtime.
    - Wasmtime if Activity is deployed and using WASM Runtime.
#### Requestor & Provider

I don't understand this chapter. Either it is obvious setup or it should be structured differently.
For example if you want to say that you can have setup where provider and requestor use the same yagna, you should say that this is an option. I wouldn't just list processes and force reader to guess.

Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dopiera and @kamirr)


arch-snapshot/arch.md line 2414 at r1 (raw file):

- Provider Agent
- Per Activity:
  - ExeUnit

It isn't clear if ExeUnit is separate binary or you describe subcomponents

Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @dopiera and @kamirr)


arch-snapshot/arch.md line 2413 at r1 (raw file):

- Yagna
- Provider Agent
- Per Activity:

Will it be understandable for reader in this part of document?

Copy link
Contributor Author

@kamirr kamirr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @dopiera and @nieznanysprawiciel)


arch-snapshot/arch.md line 2389 at r1 (raw file):

Previously, nieznanysprawiciel wrote…

I think we don't treat GSB as separate component but a way to communicate between components

Well there is the router that's which is sensible to be treated as a component. It was also listed under components in the document, and I wanted to keep it consistent.


arch-snapshot/arch.md line 2393 at r1 (raw file):

Previously, nieznanysprawiciel wrote…

Do we describe anywhere how do you build an requestor, what is it and what are the interactions?
If yes than please link it, if not it should be described

I don't really think a complete instruction to building Providers and Requestors should be in this document. We have plenty tutorials, and this isn't exactky what I'd call architecture.

@dopiera what do you think?


arch-snapshot/arch.md line 2413 at r1 (raw file):

Previously, nieznanysprawiciel wrote…

Will it be understandable for reader in this part of document?

I've reworded it to be more verbose, but otherwise assumed that a user reading about specifics of a deployment should be familiar with activities.


arch-snapshot/arch.md line 2414 at r1 (raw file):

Previously, nieznanysprawiciel wrote…

It isn't clear if ExeUnit is separate binary or you describe subcomponents

It's explicitly said that those are process trees, I didn't want to pollute it with explaining that something is a process.


arch-snapshot/arch.md line 2418 at r1 (raw file):

Previously, nieznanysprawiciel wrote…

I don't understand this chapter. Either it is obvious setup or it should be structured differently.
For example if you want to say that you can have setup where provider and requestor use the same yagna, you should say that this is an option. I wouldn't just list processes and force reader to guess.

Right, I added this for symmetry but I agree that it's somewhat confusing. Removed.

Copy link
Collaborator

@dopiera dopiera left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @kamirr and @nieznanysprawiciel)


arch-snapshot/arch.md line 2389 at r1 (raw file):

Previously, kamirr (Kamil Koczurek) wrote…

Well there is the router that's which is sensible to be treated as a component. It was also listed under components in the document, and I wanted to keep it consistent.

+1 to @nieznanysprawiciel


arch-snapshot/arch.md line 2393 at r1 (raw file):

Previously, kamirr (Kamil Koczurek) wrote…

I don't really think a complete instruction to building Providers and Requestors should be in this document. We have plenty tutorials, and this isn't exactky what I'd call architecture.

@dopiera what do you think?

+1 @kamirr - even though I find it beneficial I'm afraid we need to limit the scope of this document.


arch-snapshot/arch.md line 2380 at r2 (raw file):

* which of the logic useful to the user ends up in the SDK

## Technical view - deployment

Can we please start with some explanation of how we split the responsibilities? Feel free to refer to #109 (still unmerged). E.g. something along these lines:

Golem, as described in the layers section, stack functionalities on top of each other. As a consequence, typically there are following processes involved:

  • Yagna - the process which implements the core network, i.e. the ability for nodes to find each other and trade
  • Provider - ...
  • Requestor Agent -
  • ...

Only after you do that, please assign the components to those processes.

Code quote:

## Technical view - deployment

Copy link
Contributor Author

@kamirr kamirr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @dopiera and @nieznanysprawiciel)


arch-snapshot/arch.md line 2389 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

+1 to @nieznanysprawiciel

Done


arch-snapshot/arch.md line 2393 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

+1 @kamirr - even though I find it beneficial I'm afraid we need to limit the scope of this document.

Ack


arch-snapshot/arch.md line 2380 at r2 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Can we please start with some explanation of how we split the responsibilities? Feel free to refer to #109 (still unmerged). E.g. something along these lines:

Golem, as described in the layers section, stack functionalities on top of each other. As a consequence, typically there are following processes involved:

  • Yagna - the process which implements the core network, i.e. the ability for nodes to find each other and trade
  • Provider - ...
  • Requestor Agent -
  • ...

Only after you do that, please assign the components to those processes.

Done

@kamirr kamirr self-assigned this Nov 5, 2024
Copy link
Collaborator

@dopiera dopiera left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @nieznanysprawiciel)

@kamirr kamirr dismissed nieznanysprawiciel’s stale review November 5, 2024 20:17

Approved by Marek and comments addressed

@kamirr kamirr merged commit 40f8df3 into new-arch Nov 5, 2024
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.

3 participants