Skip to content

cmd, dashboard, eth, p2p: separate full nodes from light ones on the dashboard#19606

Closed
kurkomisi wants to merge 5 commits intoethereum:masterfrom
kurkomisi:dashboard-separate-light
Closed

cmd, dashboard, eth, p2p: separate full nodes from light ones on the dashboard#19606
kurkomisi wants to merge 5 commits intoethereum:masterfrom
kurkomisi:dashboard-separate-light

Conversation

@kurkomisi
Copy link
Copy Markdown
Contributor

Supersedes #19397

This PR contains the separation of the ETH peers from the LES peers on the dashboard.

Screenshot from 2019-05-21 19-02-34

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pls remove the sync mode, that's mostly an internal detail, is not that useful on the outside, at least not now.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pls remove empty lines from in between these imports. The only empty line should be between stdlib and externals.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pls remove the sync mode, that's mostly an internal detail, is not that useful on the outside, at least not now.

Comment thread dashboard/message.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pls remove the sync mode, that's mostly an internal detail, is not that useful on the outside, at least not now.

Comment thread eth/backend.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rather we don't add a method here. If you use admin.nodeInfo (or the equivalent call against the Eth object), you should be able to derive this whether eth, les or both protocols are advertised.

Comment thread p2p/metrics.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be Peer.Enode(), can't we get rid of this extra redundancy?

@kurkomisi kurkomisi force-pushed the dashboard-separate-light branch 2 times, most recently from 3ea7735 to abc3abb Compare June 27, 2019 11:21
Comment thread p2p/server.go
}
if remoteIP != nil {
fd = newMeteredConn(fd, true, remoteIP)
var addr *net.TCPAddr
Copy link
Copy Markdown
Contributor Author

@kurkomisi kurkomisi Jun 27, 2019

Choose a reason for hiding this comment

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

@fjl Could you please take a look? I didn't know what is the proper solution for this merge conflict.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The relevant information is that I use the TCPAddr instead of the IP.

Comment thread p2p/metrics.go
Type MeteredPeerEventType // Type of peer event
IP net.IP // IP address of the peer
ID enode.ID // NodeID of the peer
Addr string // TCP address of the peer
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this also available via Peer?

@fjl fjl removed the status:triage label Jul 18, 2019
@fjl
Copy link
Copy Markdown
Contributor

fjl commented Nov 12, 2019

closing because I am merging #19397 instead.

@fjl fjl closed this Nov 12, 2019
@karalabe
Copy link
Copy Markdown
Member

Why? This is a lot more complete

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Nov 12, 2019

Sorry, I meant I'm merging #19762 which includes this PR.

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