Improve handling of runtime servers intialization#24
Conversation
revolko
force-pushed
the
pgsubscriber-supervisor
branch
from
b99ce88 to
4aef435
Compare
revolko
force-pushed
the
pgsubscriber-supervisor
branch
from
4aef435 to
2a082dd
Compare
revolko
force-pushed
the
pgsubscriber-supervisor
branch
2 times, most recently
from
60bc53e to
8626a94
Compare
| @impl true | ||
| def handle_cast({:handle, message}, state) do | ||
| handle_message(message) | ||
| message = handle_message(message) |
There was a problem hiding this comment.
I think we are no longer handling the message, but parsing/converting it to the core message. Maybe rename?
There was a problem hiding this comment.
I would leave as is. For example, for Relation message it will be a true handling as we will have to store the state. And if we get rid of hub app (which I will do) then it makes sense.
| @@ -0,0 +1,34 @@ | |||
| defmodule Hub do | |||
There was a problem hiding this comment.
I am starting to smell delegate. Will this module do anything more? Like, why cannot this be done directly in subscriber?
Enum.each(@publishers, fn %{name: name, init_arg: _, module: publisher} ->
publisher.handle_message(name, message)
end)The communication will always be subscribers -> publishers and never other way around, right?
There was a problem hiding this comment.
True. My thinking is that hub does the spread out of a single message to all publishers. However the motivation of Hub server is the assumption that sending messages between servers is relatively hard operation. Thats why I wanted to make it a bit easier for Handlers of subscribers. Instead of copying a messages to all publishers, they would just send a message to Hub and not care anymore.
But I am starting to think that this assumption might not be correct. I will read about it a bit. If it turns out that it is incorrect then I will move this logic to Handler. But if it holds, then IMO it is valid point.
There was a problem hiding this comment.
idk, Handler already has the message, it does not copy. It, it would just call (asynchronnaly, without waiting) M publishers instead of calling one Hub.
| defp get_children_spec(config_properties) do | ||
| Enum.map(config_properties, fn property -> | ||
| case property do | ||
| module_name when is_atom(module_name) -> |
There was a problem hiding this comment.
Hmm, I wanted name to be optional. Now I have 2 options: either I fill in everything, or only the module name. How do I define a single PgSubscriber (without name parameter) when it always needs database connection parameters?
There was a problem hiding this comment.
Yeah making name optional makes more sense... Since init args where hardcoded I completely forgot about them. 😄
There was a problem hiding this comment.
I have changed so the name is actually never required. Thanks Registry. 😄
|
Maybe it is just because of my recent problems with it (a coworker marking my comments as completed even though they were not), but I think the conversation should be resolved by the one who requested the changes. Now I had to expand all conversations to check what we were discussing and whether you changed in the way I imagined. From quick search here and here it seems that internet agrees with me as well as AI:
|
Fair enough. I agree with you. |
The idea is that each subscriber will initialize its own Supervisor tree. That way we can make sure that the replication client is configured with the same handler and that they are intialized in the correct order. It will basically make the configuration a bit more straighforward. Note, we need to rething the supervisor child strategy. Currently, if you try to configure multiple PG subscribers the second one will not initialize because of naming collisions for Repl and Handler servers. Relates: #20
That way we can start multiple FilePublisher servers at the same time. ---v2 Not defining name allows use to create multiple instance of FilePublisher without worrying about naming collisions. The messages will be send based on the PID of a process.
revolko
force-pushed
the
pgsubscriber-supervisor
branch
from
8626a94 to
5c94dd0
Compare
Processes can register themselves to the Registry. In our case, we need to register publishers. The registry stores PIDs of registered servers and can store some other information. E.g. information about which function to trigger when calling `Registry.dispatch/3`. That is very convenient way of defining a generic entry to publisher (publishers define which function is called during dispatch; it just needs to follow specification defined in the PublisherContract).
revolko
force-pushed
the
pgsubscriber-supervisor
branch
from
5c94dd0 to
94efbab
Compare
|
I have created issue to figure out configuration validation #26 since I don't like the way it is done right now. However, during development I think it is fine. |
2 participants
No description provided.