-
Notifications
You must be signed in to change notification settings - Fork 100
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
Process iterator in RPC result #583
Conversation
If you return the values of the iterator directly, the developer will not be able to distinguish whether the method returns an array or an iterator. |
I think that api users doesn't care about the low level type, they want the value. Otherwise you will need to know first that it's an iterator result in order to make a different call. |
At least we should not modify its type.
|
I tried modules, making everything to config is not a good way, and server can't modify config when it's running a node. I find more and more paramaters are added to config.json, it's too complex for reading and understanding for a new developer. I like the style of #577 much more. Should give rights to user but not limit it from server, that's why choose iteratror but not list, right? |
As @satoshichou said, I totally agree. Don't make config too heavy and user could choose how much times iterator could run. |
Anyway an output limit it's a good choice.
The cost of this "feature" it's to choose a different method if you call an specific one, harder for developers, there are any use case for return an specific index? the server will process all of them in order to fetch them, this process can be delegated to the client. |
I agree with @superboyiii and @satoshichou and I prefer that Pull Request |
I support @shargon's solution. Because if you use index, it may cause data inconsistency. For example, if I call |
I'm confused about |
Co-authored-by: Satoshi Chou <82646971+satoshichou@users.noreply.github.com>
There's a boundary case. If the value of |
{ | ||
json["warning"] = warning; | ||
} | ||
json["stack"] = new JArray(engine.ResultStack.Select(p => ToJson(p, settings.MaxResultItems))); |
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.
Now is not a MaxResultItems
it's a MaxIteratorResultItems
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.
Yes. We should rename it.
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.
Test pass, just need to rename the parameter.
Since this solution not supports index, when the amount of data is very large, does user need to run his own node to get the complete data? |
Yes. I think public nodes are not suitable for running high-load tasks. |
Alternative to #577