Skip to content

refactor: remove GenServer behavior from gossip blobsidecar #1115

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

Merged
merged 11 commits into from
Jun 7, 2024

Conversation

avilagaston9
Copy link
Contributor

@avilagaston9 avilagaston9 commented May 29, 2024

This PR removes all the GenServer behavior from the gossip BlobSideCar.

Closes #1114

@avilagaston9 avilagaston9 changed the title refactor: gossip blobsidecar as a Task refactor: Remove GenServer behavior from gossip blobsidecar Jun 3, 2024
@avilagaston9 avilagaston9 changed the title refactor: Remove GenServer behavior from gossip blobsidecar refactor: remove GenServer behavior from gossip blobsidecar Jun 3, 2024
@avilagaston9 avilagaston9 marked this pull request as ready for review June 4, 2024 15:57
@avilagaston9 avilagaston9 requested a review from a team as a code owner June 4, 2024 15:57
Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

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

1 comment about error handling. The rest is fine.

@spec join_topics() :: :ok
def join_topics() do
topics()
|> Enum.each(fn topic_name -> Libp2pPort.join_topic(self(), topic_name) end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not use each if it can return an error and leave the node in an unusable state. We should be able to return an error here, for example, with a reduce, or with a map and then counting the errors.

@avilagaston9 avilagaston9 marked this pull request as draft June 6, 2024 19:46
@avilagaston9 avilagaston9 marked this pull request as ready for review June 7, 2024 13:26
Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

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

LGTM with small comment

Comment on lines 33 to 34
Libp2pPort.subscribe_to_topic(topic, __MODULE__)
|> case do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's avoid the pipe for a single operation.

case Libp2pPort.subscribe_to_topic(topic, __MODULE__) do
 :ok -> ...
 {:error, reason} -> ...
end

@avilagaston9 avilagaston9 merged commit 2c4f4d2 into main Jun 7, 2024
13 checks passed
@avilagaston9 avilagaston9 deleted the refactor-blobsidecar-gossip branch June 7, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[Refactor] Remove all GenServer behavior from BlobSideCar gossip
2 participants