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

Include horde_process in the main repo #272

Closed
caioaao opened this issue Jun 24, 2024 · 12 comments · Fixed by #273
Closed

Include horde_process in the main repo #272

caioaao opened this issue Jun 24, 2024 · 12 comments · Fixed by #273

Comments

@caioaao
Copy link
Contributor

caioaao commented Jun 24, 2024

https://github.com/tyler-eon/horde-process

This seems very useful, specially for initial adoption. It's a single file defining a behaviour + implementation. At the very least the documentation @tyler-eon wrote to their lib was very helpful in understanding issues we were having with Horde in production

@sleipnir
Copy link

https://github.com/tyler-eon/horde-process

This seems very useful, specially for initial adoption. It's a single file defining a behaviour + implementation. At the very least the documentation @tyler-eon wrote to their lib was very helpful in understanding issues we were having with Horde in production

I think we could have an api built into Horde this way. As long as this is completely optional I believe it is a good thing.

@sleipnir
Copy link

Another important point would be to add an option for the call and cast functions, allowing you to bypass the lookup step. Sometimes it is interesting to take the risk and use other retry or failure handling mechanisms if the target process has not been completely synchronized in CRDT

@tyler-eon
Copy link

Another important point would be to add an option for the call and cast functions, allowing you to bypass the lookup step. Sometimes it is interesting to take the risk and use other retry or failure handling mechanisms if the target process has not been completely synchronized in CRDT

Always looking for feedback, thanks @sleipnir ! Quick question though, what do you mean by "bypass the lookup step"? Do you mean effectively calling GenServer.cast/call directly? If you don't have a way to accurately construct a PID reference, you would need to use a via tuple. You could theoretically get that by calling MyProcess.via_tuple/1 and pass that as the server name argument in GenServer.cast/call. However I'm not sure if that would give you any benefit because the via tuple references the Horde Registry, and so if it hasn't been added to the node-local process registry yet then it doesn't matter whether you use something like fetch/1 or try to call GenServer directly using via_tuple/1.

Apologies if I've misunderstood the request.

@derekkraan
Copy link
Owner

Let's start with linking to the project in the README?

@sleipnir
Copy link

Always looking for feedback, thanks @sleipnir ! Quick question though, what do you mean by "bypass the lookup step"? Do you mean effectively calling GenServer.cast/call directly? If you don't have a way to accurately construct a PID reference, you would need to use a via tuple. You could theoretically get that by calling MyProcess.via_tuple/1 and pass that as the server name argument in GenServer.cast/call. However I'm not sure if that would give you any benefit because the via tuple references the Horde Registry, and so if it hasn't been added to the node-local process registry yet then it doesn't matter whether you use something like fetch/1 or try to call GenServer directly using via_tuple/1.

Apologies if I've misunderstood the request.

If we had a reference to the PID somewhere in the api we wouldn't have to do the lookup

@caioaao
Copy link
Contributor Author

caioaao commented Jun 26, 2024

Let's start with linking to the project in the README?

I'd go further and include some of their docs in the readme as well! Their explanation on the init stuff, plus the retries were the missing pieces for us (and we've been running horde in prod for nearly a year afaik)

@derekkraan
Copy link
Owner

Let's start with linking to the project in the README?

I'd go further and include some of their docs in the readme as well! Their explanation on the init stuff, plus the retries were the missing pieces for us (and we've been running horde in prod for nearly a year afaik)

This is an open source project, PRs improving the documentation are welcome.

@caioaao
Copy link
Contributor Author

caioaao commented Jun 26, 2024

Let's start with linking to the project in the README?

I'd go further and include some of their docs in the readme as well! Their explanation on the init stuff, plus the retries were the missing pieces for us (and we've been running horde in prod for nearly a year afaik)

This is an open source project, PRs improving the documentation are welcome.

I know (and I usually do), butI'll sit this one out and I'm just relaying an info I found useful exactly because I lack understanding of the lib's internals, quirks, etc, so I don't think I'm the best to update the docs, and I thought the maintainer would know best what to do with that info 🤷

@tyler-eon
Copy link

If we had a reference to the PID somewhere in the api we wouldn't have to do the lookup

Ah, I see. Well, you actually can skip the lookup and just use GenServer.call and GenServer.cast directly with the PID since a Horde Process is just a GenServer right now.

# Using a Horde Process might look like...
My.Horde.Process.call(identifier, {:foobar, args})

# But if you already have the PID, skip it and use GenServer directly
GenServer.call(pid, {:foobar, args})

In fact, if you look at the source for e.g. call!/3, you'll see that it's just two lines of code: one to get the PID and one to invoke GenServer.call/3. So if you already have the PID itself then yeah you can, and certainly should, skip using the helper functions.

@tyler-eon
Copy link

Let's start with linking to the project in the README?

I'd go further and include some of their docs in the readme as well! Their explanation on the init stuff, plus the retries were the missing pieces for us (and we've been running horde in prod for nearly a year afaik)

This is an open source project, PRs improving the documentation are welcome.

It feels a bit self-aggrandizing if I were to link my own project in your readme, but I will at least see if I can add to the documentation some of the pitfalls I ran into that shaped the direction of my init function (especially the wait_for_init stuff).

@sleipnir
Copy link

sleipnir commented Jun 27, 2024

If we had a reference to the PID somewhere in the api we wouldn't have to do the lookup

Ah, I see. Well, you actually can skip the lookup and just use GenServer.call and GenServer.cast directly with the PID since a Horde Process is just a GenServer right now.

# Using a Horde Process might look like...
My.Horde.Process.call(identifier, {:foobar, args})

# But if you already have the PID, skip it and use GenServer directly
GenServer.call(pid, {:foobar, args})

In fact, if you look at the source for e.g. call!/3, you'll see that it's just two lines of code: one to get the PID and one to invoke GenServer.call/3. So if you already have the PID itself then yeah you can, and certainly should, skip using the helper functions.

I think I didn't explain my point well. What I wanted to say is that since you are providing an API, it should provide the means without having to resort directly to a lower-level API, in the case of GenServer.

See this example from the Spawn project:
https://github.com/eigr/spawn/blob/51222e6ca78377411674bfd64c98d165371df45f/spawn_sdk/spawn_sdk/lib/actor.ex#L100

Do you realize that the API itself deals with passing on the reference? It's just an example but it explains my point of view a little better, I think.

But I think this discussion could be moved to the library repository in question so as not to disturb this thread here on Horde.

caioaao added a commit to caioaao/horde that referenced this issue Jun 27, 2024
@caioaao
Copy link
Contributor Author

caioaao commented Jun 27, 2024

Let's start with linking to the project in the README?

I'd go further and include some of their docs in the readme as well! Their explanation on the init stuff, plus the retries were the missing pieces for us (and we've been running horde in prod for nearly a year afaik)

This is an open source project, PRs improving the documentation are welcome.

It feels a bit self-aggrandizing if I were to link my own project in your readme, but I will at least see if I can add to the documentation some of the pitfalls I ran into that shaped the direction of my init function (especially the wait_for_init stuff).

That I can do: #273

derekkraan pushed a commit that referenced this issue Jun 27, 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 a pull request may close this issue.

4 participants