WIP: Split LCD implementation PR, part two #2199
Conversation
Split LCD implementation PR, part two
HaoyangLiu
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
|
The following PRs mentioned before about other APIs and spec will depend on this PR. It would be wonderful if this PR could be reviewed and merged soon. |
HaoyangLiu
changed the title
| @@ -0,0 +1,46 @@ | |||
| package context | |||
There was a problem hiding this comment.
This is interesting, but with only round-robin load balancing it's not that useful in my opinion. I would rather have a discussion about what we want this to do for clients - maybe track failed requests, or track latency, and automatically switch clients to provide the best UX. I'm not convinced we need this right now (but if you think we do by all means explain why).
There was a problem hiding this comment.
The round-robin load balancing is indeedly simple. But it does contribute to TPS, please refer to the my test report. I think a load balancing module is necessary. Maybe currently we can just remove this module. But in long run, we have to add load balancing. How do you think about this?
There was a problem hiding this comment.
I think it depends on the use-case that the this client/context is designed for. As is it's optimized for a single wallet application to be bundled w/ Electron in the Voyager app... if we're making calls to query the app state then ultimately a full node is required to service the state queries (with or without proof), so it would arguably work to just have load-balancing behind a set of full nodes.
| @@ -0,0 +1,725 @@ | |||
| // GENERATED BY THE COMMAND ABOVE; DO NOT EDIT | |||
There was a problem hiding this comment.
Is this from a previous consensus on API documentation - can you link to it?
There was a problem hiding this comment.
This is something that has been discussed repeatedly in a number of issues. Having automatically generated API docs is going to be increasingly important as the surface area of the API grows. I've also personally used swagger in previous projects and found it amazing.
There was a problem hiding this comment.
Generally agreed - but we should figure out how exactly we want to do this. If we can now generate these automatically:
- Are the downstream consumers (@faboweb @fedekunze) in favor of this particular autogeneration format / will it suit their needs?
- Why does this PR commit an autogenerated file?
- Can we remove the existing manual docs in favor of autogenerated ones?
There was a problem hiding this comment.
- Yes, see issues
- It shouldn't, we should add that to
.gitignore - Yes, eventually.
There was a problem hiding this comment.
Great, thanks for the clarification. Sounds like @faboweb or @fedekunze would be far better suited than I to review this PR!
There was a problem hiding this comment.
@cwgoes @jackzampolin @fedekunze
Please refer to this link for swag tool. Currenly it has some defects. It can't properly generate the data structure for some complex self-define struct. Besides, I can find all APIs when theses API don't locate in a same root folds. So I have to manually edit docs.go
There was a problem hiding this comment.
I don't think we want to be in a position where we are relying on a non functional tool and manual editing for our docs process. Can't we just use a swagger.yaml file and update that instead of relying on go struct tags and the swag tool? Or alternately can we try to figure out how to get the generation to work properly? Would love to hear some other thoughts here.
There was a problem hiding this comment.
Please refer this swag example. It requres the main.go locate in the root fold. And all related handlers and data struct also locate in this root folds. However, in cosmos-sdk, main.go locates in cmd/gaia/cmd/gaiacli, and handlers locate in many folds, such as client/key, x/. And data strcut locates in many folds too.
So if we want to make swag generation works well, we have to refactor our code structure. Do you agree with me @fedekunze ?
I'm thinking usging swagger.yaml file. But it may also require maunally edit API docs.
In long run, maybe refactor our code strcuture is a good idea.
|
CC: @cosmos/cosmos-ui |
|
@cwgoes @jackzampolin |
|
Please move to #2215 |
HaoyangLiu
changed the title
Proceed from:
#2118
#2147
#2177
Closes #525 and #1020 and #2179 and #2108