-
Notifications
You must be signed in to change notification settings - Fork 925
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
feat(cmd/rpc): Automatically detect running node for RPC requests #3246
feat(cmd/rpc): Automatically detect running node for RPC requests #3246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the pr!
Lets not complicate things and stick to the former but with full swaped with light in the order.
In the case that multiple node types are running on 1 network, the request is directed to 1 node in this order: bridge, light, full. Can change that ordering or alternatively, can detect many node types and ask user to clarify via CLI input. I like former approach since this use case seems uncommon.
This PR is prompted by #3246. While looking through the code and understanding how `IsLocked` worked there, I realized that our fslock lib was missing a feature. Instead of asking @mastergaurang94 to fix it, I decided to completely migrate over to a more [comprehensive locking solution](https://github.com/gofrs/flock), which additionally: * avoids the need custom code to maintain(NIHS) * solves the problem of locking being annoying after an ungraceful shutdown * gives us Windows and other niche OS support * as well as works around niche FS edge-cases
…urang94/celestia-node into gaurang.rpc-detect-node
@Wondertan thanks for comments! Updated also, note that flock now leaves .lock file in directory - https://github.com/gofrs/flock/blob/master/flock.go#L54 |
I am aware and that's ok. The locking functionality should be around the flock syscall, not the existence of the file. |
… into gaurang.rpc-detect-node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this forward and writing good tests!
One minor comment and one major comment about logic improvement
This PR is prompted by celestiaorg#3246. While looking through the code and understanding how `IsLocked` worked there, I realized that our fslock lib was missing a feature. Instead of asking @mastergaurang94 to fix it, I decided to completely migrate over to a more [comprehensive locking solution](https://github.com/gofrs/flock), which additionally: * avoids the need custom code to maintain(NIHS) * solves the problem of locking being annoying after an ungraceful shutdown * gives us Windows and other niche OS support * as well as works around niche FS edge-cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one small nit on formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this great contribution and for persistence on resolving multiple requested changes!
Haven't seen the approval in a long time 🥹 Thank you for lending your time & energy to this review @Wondertan! Had fun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Thank you 🚀
cc @jcstein for visibility |
Cross-posting from slack: This flag also appears:
these are the two that i found on a quick search what impact does this have on |
@jcstein For non-default path users, they have to specify the path using the flag. |
Got it, thanks @Wondertan. If I were running with a non-default node store, making an RPC request, I would still use the node.store flag, correct? |
Correction after looking at the code. cel-key does have automatics but they aren't as smart as in this PR and this PR doesn't chang those |
Yes |
gm @mastergaurang94 - I'm working on the docs for this. can you help me understand which network(s) you were running a node(s) for on for 4-6?
I'm assuming you had a mocha node running, the request was sent to mainnet node, but looks like the call failed?
I'm guessing you had mocha and arabica running?
It looks like this request fails? |
hey @jcstein, sure:
main thing, node types are unique. these were the cases I listed but any variation will work
|
awesome, thanks for the details here @mastergaurang94 could you please take a look at the corresponding docs PR? |
Closes #2859.
TL;DR Previously, the node.store flag had to be specified manually for each RPC request. This commit introduces automatic detection of the running node.
Assumptions:
Error: node: store is in use
).Sample Test Cases