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

Allow visor to start in case of address resolver or tp disc failure #402

Merged
merged 18 commits into from
Jun 17, 2020
Merged

Allow visor to start in case of address resolver or tp disc failure #402

merged 18 commits into from
Jun 17, 2020

Conversation

Darkren
Copy link
Contributor

@Darkren Darkren commented Jun 11, 2020

Did you run make format && make check?
Yes
Fixes #401

Changes:

  • Visor won't start before it connects to TP disc. it keeps dialing TP disc till sucess
  • Added callback to notify outer-network code when new network gets established
  • Fixed transport manager to handle background-initiated networks
  • Fixed already-known transports initiation

Tested partially, except for the transport manager initialization and already-known transports. We need to have stcpr/stcph transports working to test this. Also code may look clumsy in some places, but this should get sorted out as soon as we deal with the refactoring of network part which is proposed by @nkryuchkov

@Darkren Darkren changed the title [WIP] Allow visor to start in case of address resolver or tp disc failure Allow visor to start in case of address resolver or tp disc failure Jun 13, 2020
@Darkren Darkren requested review from nkryuchkov and evanlinjin and removed request for nkryuchkov June 13, 2020 16:52
Copy link
Contributor

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Probably needs further discussion.

addressResolver = ar
if conf.NetworkConfigs.STCPR != nil || conf.NetworkConfigs.STCPH != nil {
var (
addressResolver arclient.APIClient
Copy link
Contributor

Choose a reason for hiding this comment

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

When I first made the snet library, I intended it to be just a wrapper for all the network types. I think it would be cleaner if we didn't have any reconnecting logic here at all. Would we be able to implement the reconnect logic within arclient.httpClient?

Copy link
Contributor

Choose a reason for hiding this comment

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

@evanlinjin @Darkren Would you mind if I work on the reconnection logic of arclient.httpClient? Its code is familiar to me and I have some ideas for reconnection logic. Moreover, I'm going to add a UDP connection in #400. In my opinion, it would make more sense to implement reconnection logic after UDP implementation is finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nkryuchkov I'll leave a TODO comment then

pkg/snet/network.go Outdated Show resolved Hide resolved
pkg/snet/network.go Outdated Show resolved Hide resolved
pkg/transport/manager.go Show resolved Hide resolved
pkg/transport/manager.go Show resolved Hide resolved
pkg/snet/network.go Outdated Show resolved Hide resolved
pkg/snet/network.go Show resolved Hide resolved
pkg/transport/manager.go Outdated Show resolved Hide resolved
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

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

Works fine. Can be merged if @evanlinjin is happy to fix the rest in another issue tackled by nikita.

Copy link
Contributor

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Good job! Looks like it is ready to merge.

@jdknives jdknives merged commit e4df449 into skycoin:develop Jun 17, 2020
jdknives added a commit that referenced this pull request Oct 19, 2020
…hout-tp-disc-and-addr-resolver

Allow visor to start in case of address resolver or tp disc failure

Former-commit-id: e4df449
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.

4 participants