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

Add API framework decouple proposal #17

Merged
merged 7 commits into from
Mar 19, 2019
Merged

Add API framework decouple proposal #17

merged 7 commits into from
Mar 19, 2019

Conversation

leonwanghui
Copy link
Collaborator

No description provided.

Signed-off-by: leonwanghui <wanghui71leon@gmail.com>
@stmcginnis
Copy link
Contributor

I really like the idea. I mentioned some spelling nits inline, but otherwise this proposal looks like a great idea to me.

Signed-off-by: leonwanghui <wanghui71leon@gmail.com>
specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
}

message GenericResponse {}
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have Get and List methods for each resource to check the status of an operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One question for these methods, since we can check the status of operation through etcd, is there any need to double-check the status by calling the backend?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a QoS parameter into the design like some messaging implementations.
Level 1: no check
Level 2: check backend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@noelmcloughlin Good suggestion, but actually in the long term we want to redesign a unified model for both controller and dock module.


##### Pros
Decoupling `api-server` from `controller` would make `api-server` much easier to scale out, and therefore improving
performance when handling heavy workloads.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does allow api-server to scale out, but I don't think this improves performance as it adds another layer of indirection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I mean here is that for heavy workloads, only one api-server will cause the traffic delay for the request, but as for multiple api-server modules all requests will be split and therefore the overall time would be reduced. Does that make sense?

specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
specs/capri/API_Framework_Refactoring.md Outdated Show resolved Hide resolved
@xing-yang
Copy link
Collaborator

Can you add a diagram to show this change?

@@ -82,7 +82,7 @@ separate PR after the items above are resolved.

### Data model impact

Because grpc model requires generated from protobuf, so some protobuf files should be added:
Because grpc model requires generated codee from protobuf, so some protobuf files should be added:
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/codee/code

Signed-off-by: leonwanghui <wanghui71leon@gmail.com>
@xing-yang
Copy link
Collaborator

@leonwanghui can you address the comments? After that I'll merge this.

@xing-yang
Copy link
Collaborator

1 similar comment
@xing-yang
Copy link
Collaborator

@xing-yang
Copy link
Collaborator

lgtm

@xing-yang xing-yang merged commit 6c7b0c7 into sodafoundation:master Mar 19, 2019
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.

4 participants