replace own worker address by localhost and do not end gossip loop when peer connections fails#601
replace own worker address by localhost and do not end gossip loop when peer connections fails#601gaudenzkessler wants to merge 2 commits intomasterfrom
Conversation
gaudenzkessler
commented
Dec 27, 2021
gaudenzkessler
commented
- Replace in list of peers the own address of the worker itself by ws://localhost.
- Do not end gossip block loop when the connecting to peer fails.
| for p in peers.iter() { | ||
| // Todo: once the two direct servers are merged, remove this. | ||
| let url = worker_url_into_async_rpc_url(&p.url)?; | ||
| let url = match worker_url_into_async_rpc_url(&p.url) { |
There was a problem hiding this comment.
That will be made obsolete in PR #595 - but it will be easy to remove the conflict, so I don't mind.
There was a problem hiding this comment.
I think this part in the gossiping loop has been made obsolete indeed, by more than one PR. So these changes here are not needed anymore.
| Ok(()) | ||
| } | ||
|
|
||
| /// Returns a list of peers. The address of the worker self is replaced by ws://localhost. |
There was a problem hiding this comment.
Could you add a note why this is necessary? Because for non-docker usage this doesn't make sense.
There was a problem hiding this comment.
That would be of interest for me as well, because it explains why we have this second change in the PR 😄 (which seems to be not obvious for a number of people)
| if e.url.trim_start_matches("ws://").trim_start_matches("wss://") | ||
| == self._config.worker_url() | ||
| { | ||
| e.url = format!("ws://localhost:{}", self._config.worker_rpc_port); |
There was a problem hiding this comment.
hm - hardcoded ws? Is it not possible to replace 127.0.0.1 or whatever ip with localhost , instead of replacing it fully?
There was a problem hiding this comment.
I agree with @haerdib that this could and should be simplified. If a string contains our worker url, replace it with localhost:port. We don't have to care about the protocol (like ws:// or wss://) at all (and shouldn't). Also I'd like to see some unit tests 😈
|
Is this still needed after @haerdib changes? |
|
Once again: @gaudenzkessler , @haerdib isn't this one obsolete? |
|
I doubt that |
|
It seems nobody knows what should happen with this PR? According to @gaudenzkessler , @clangenb should know - but that does not seem to be the case? Do we need to schedule a meeting and discuss this? |
|
Lets discuss it quickly on the next daily. And if we cannot clarify, we will schedule another meeting. |
murerfel
left a comment
murerfel
left a comment
There was a problem hiding this comment.
I think we can completely drop the first part of changes in this PR, and improve on the implementation of the second part (url replacement with localhost) - as noted in the comments.
| for p in peers.iter() { | ||
| // Todo: once the two direct servers are merged, remove this. | ||
| let url = worker_url_into_async_rpc_url(&p.url)?; | ||
| let url = match worker_url_into_async_rpc_url(&p.url) { |
There was a problem hiding this comment.
I think this part in the gossiping loop has been made obsolete indeed, by more than one PR. So these changes here are not needed anymore.
| Ok(()) | ||
| } | ||
|
|
||
| /// Returns a list of peers. The address of the worker self is replaced by ws://localhost. |
There was a problem hiding this comment.
That would be of interest for me as well, because it explains why we have this second change in the PR 😄 (which seems to be not obvious for a number of people)
| if e.url.trim_start_matches("ws://").trim_start_matches("wss://") | ||
| == self._config.worker_url() | ||
| { | ||
| e.url = format!("ws://localhost:{}", self._config.worker_rpc_port); |
There was a problem hiding this comment.
I agree with @haerdib that this could and should be simplified. If a string contains our worker url, replace it with localhost:port. We don't have to care about the protocol (like ws:// or wss://) at all (and shouldn't). Also I'd like to see some unit tests 😈
5 participants
