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

light-client: add missing RPC endpoint and extend handler/supervisor #449

Merged
merged 15 commits into from
Jul 16, 2020

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented Jul 15, 2020

#219 (comment)
and contains a preliminary fix for #450 (thanks @OStevan)
closes #450
closes #219

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

@liamsi liamsi requested review from xla, romac, brapse and ebuchman July 15, 2020 20:33
@liamsi
Copy link
Member Author

liamsi commented Jul 15, 2020

TODOs:

  • add tests
  • update light-node Readme
  • changelog?

 - from `synced to block {} 1234` to `synced to block: 1234`
light-node/src/rpc.rs Outdated Show resolved Hide resolved
@liamsi liamsi marked this pull request as ready for review July 16, 2020 13:43
CHANGES.md Outdated Show resolved Hide resolved
brapse
brapse previously approved these changes Jul 16, 2020
Copy link
Contributor

@brapse brapse left a comment

Choose a reason for hiding this comment

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

🎉 🚢 💡 💁 💼

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Dope! Very straighforward extension.

The issue I see is that we make the entire LatestStatus optional. Which for my understanding of an operational endpoint is confusing and actually reducing the signal, as for the caller it's unclear what the reason might be, as the system only returns one when there is a primary with a trusted block. Instead we should always return a proper response and make fields in there optional. This part should be addressed in this PR IMHO.

Beyond that the real value of this new endpoint comes from the peer list, the other info can be obtained from the state endpoint. We would benefit by reworking the status one into giving operational insight, like what's the state known for a given peer, latest block, status of latest block and if they are our current primary. In a very far future this could expose the different lists in PeerList. We can look into that in follow-ups.

@liamsi
Copy link
Member Author

liamsi commented Jul 16, 2020

The issue I see is that we make the entire LatestStatus optional.

I think the reason is exactly the same as why the endpoint you created is optional too but yeah, you are right in this context it is important to return any information if it is available. Do you think that all these fields should basically be optional then?

@liamsi liamsi requested a review from xla July 16, 2020 16:17
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

👂 ⛰ 🆘 🏃

@romac
Copy link
Member

romac commented Jul 16, 2020

Looks great! 👍

Just a quick note on the new LightStore::latest method: self.db(status).iter(&self.db).max_by does a full linear scan of the database, so this might perform very badly as the light store grows. Not a blocker for this PR, but just something to keep in mind for when we revamp the LightStore. Will add a reference to this comment to #428.

@brapse brapse merged commit 02fa831 into master Jul 16, 2020
@brapse brapse deleted the ismail/rpc_add_status_endpoint branch July 16, 2020 16:34
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.

light-client: light store does not seem to update Light Client RPC Service
5 participants