-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
refactor: runtime takes no dependencies on capabilities #240
Conversation
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
…me crate Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
8e9cdac
to
0add22a
Compare
@Mossaka: I'd like to know what the perf difference is at startup. When you say there's a tradeoff, I wanna know what it is.... |
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.
Definitely a wide reaching PR, but one that I'm glad to see materialize. I've only added some trivial comments about removing some commented code. Outside of that, it lgtm!
I believe the tradeoff is not runtime impacting, but rather code design impacting. It makes the CLI need to know a bit more about the capabilities. I believe this is a preferable tradeoff when weighed against the runtime needing to know details about the capabilities. |
OK, perfect. I saw the internal chatting after commenting. Sounds great to me! |
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.
mostly lgtm~ There's one thing I think you missed, and I want to address the possible removal of some error handling that we had previously before merging this in.
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.
@Mossaka I'm not too familiar with this codebase but overall, looks good to me. I think some documentation on at least the newly introduced pub
types would be good.
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
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.
LGTM~ cool workspace feature usage! ;D
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
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.
LGTM! Great work, @Mossaka!
I have to admit that this is a relative large PR and apologize beforehand. It refactors the core of slight - the runtime crate to not depend directly on any of the slight capabilities, including slight-kv, slight-mq, etc. This comes at a cost though. The slight CLI implementation now needs to know how to constrct a capability service before adding it to the slight runtime builder, which is a acceptable compromise.
In addition to the runtime refactoring, this PR also cleans up some redudent code in slight capabilities. Particularly, this PR removed capability-state.
Signed-off-by: Jiaxiao Zhou jiazho@microsoft.com