-
Notifications
You must be signed in to change notification settings - Fork 246
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
Add a call to wait for the node to be fully functional. #8
Comments
FYI: We have a bitcoin-rpc-client that can already do this. Maybe you can reuse it :) The problem is that you cannot determine with a single call, whether the node is fully functional because different parts of the node are ready at different times. That is why we ended up retrying a each call for a given number of times until the specific error code ( |
That's useful. I think we could do something similar. |
There's two approaches here:
I think personally the latter gives more freedom to the users. But it's a bit more cumbersome as it might require a few lines of extra boilerplate. |
I think the In my experience node quite often will respond with just empty string, example when it's under too heavy load. Or the node can be temporarily turned off for maintenance/upgrade. Which in practice means that whatever system is build on top of the node JsonRpc API, should handle all sort of failures in a higher level layer. So that |
I have to disagree. If you take a closer look on how we implemented it, you can see that it doesn't actually introduce a performance penalty. Immediately successful calls will return the result just as if there was no error handling. We initially tried to go down the
That is why we ended up handling only the The result is, if you take the "callers should handle errors" (which I totally agree with) to the "next level", you end up at: what will be the error handling strategy employed by the caller? If it is not yet ready, chances are you will want to try again. Just letting it bounce up to the caller only has downsides IMO. If you don't like it for production, just turn it off through the configuration and during testing, it just works :) |
It has nothing to do with performance.
The error handling strategy is whatever is necessary. Chances are that it is not going to be retrying, and then what? My thread might have something better to do, or I eg. I might be doing software implemented round-robin over a pool of redundant bitcoin core nodes. The rule of thumb for software composability is: it's easier to add the missing bits, than remove the unnecessary ones. With retyring, now you have to document which calls are going to be retried, and which not. Under which conditions exactly. How much retries, how long is the delay, is there exponential backoff implemented and so on. Users might overlook it, or get something wrong, or it might just not be what they need. If anyone wants retrying, they can always write a simple adapter that adds retrying on top of this library with no downsides. |
I'd argue that it is a very common problem and that a library which sort of claims to be the "official" bitcoin-rpc client for the Rust community should provide a solution for it.
I agree with both statements. Maybe the conclusion is that this library should just provide such an adapter? At the moment, the implementation doesn't expose a trait for the possible RPC calls. If we would introduce one, providing an adapter should be straight forward and users could compose things together as they need it. |
I am all 👍 for an adapter in the library itself. |
Hmm, one way I see this adaptor be possible is to define a trait with the RPC methods and with Then This would require the user to import both the trait and the Thought? |
There's nothing that is 100% needed. Anyone can just add a struct wrapping the There could be a In my code I usually define my own |
I think the value would be more in what you mentioned in the 2nd paragraph: Using the trait to create test doubles. If you have got your own trait, it is unnecessary but it is cumbersome if you have to define one even though you wouldn't need it otherwise.
I like this approach.
We need it :D
I think most crates in the ecosystem solve problems like these with a custom |
You should define the smallest possible interface you need and not throw everything and kitchen-sink into one common trait. There is like 0% chance that whatever you're building is going to use all these call. So why would you want to use such trait as API for testing? Just to confuse yourself and everyone in which calls really have to be mocked? If your app/component/whatever only calls |
That is the ideal scenario, I agree but I would say that going for the ideal scenario is not always worth it.
To make my life simpler and having to care about less stuff like needing to define my own trait before I can do any kind of mocking. In my opinion a library should not constrain its users in the way it can be used as long as the result is still correct. It should be open to as many usecases as possible and I'd say it is quite common to represent an interface to any kind of thing in an adapter library with whatever abstract construct the language provides, like What would be the downside of including such a trait in the this library? |
What's not worth it is maintaining ill-conceived traits in common libraries. It's just another API surface to confuse users and additional chore for the maintainers. All in the name of some hypothetical convenience for hypothetical (and lazy) user that can't do something right for their own good. First, the idea of retrying inside this library has bunch of problems, and just enables people to write buggy software that does not have a reliable handling of errors. Because ill-conceived convenience. Then, a trait is consider, which has problems, and just enables people (hypotetically) to do their abstractions wrong. Again, because ill-conceived convenience. Then maintainers will get tired of fixing typos and documentation in 3 places, so someone will add procedural macros "to fix it". Because ill-conceived convenience. From a small library with tiny and well defined purpose, we land with over-complicated monster, that is hard to maintain, review and confusing for users. I really have no time for this. You're free to implement it and whatever other ideas you'd like in a library wrapping this library, or other owners of this crate can go along with it if so they like. As long as I don't have to do anything, it's fine with me. :) Now, to stick to the subject of the issue: what would be useful is to have a call that checks if the node is ready in a reliable way, so users don't have to google around to figure out how to do it. |
Take a look here guys: #15 |
Something like this should be sufficient to have an auto-retry client: |
Sounds great! |
@D4nte did you take a look at my proposal? |
No description provided.
The text was updated successfully, but these errors were encountered: