-
Notifications
You must be signed in to change notification settings - Fork 16
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
Tweaked invoker and added tests #540
base: main
Are you sure you want to change the base?
Conversation
I don't see the tests for the driver/clients actually running in the CI. Can you please check the workflow? Also, there is a linter error, please fix. |
Please squash your commits, sign off, and rebase the branch on current main. |
39cdf02
to
4c372fb
Compare
Hi, I've rebased on main and squashed and signed off my commits. I've also fixed the Dirigent mode to Knative mode. Please check |
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.
Please rebase and fix the indentation in pkg/driver/clients/grpc_client.go
197fbbc
to
af79127
Compare
af79127
to
df997aa
Compare
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.
Please fix the failing linter test and look at the comment about test VSwarm server once again.
c447b94
to
46a4c3b
Compare
46a4c3b
to
f3b274d
Compare
f3b274d
to
cdd2a17
Compare
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.
Please fix minor things
@cvetkovic when Aryan does, please take a look as well
type vHiveMetadata struct { | ||
WorkflowId string `json:"WorkflowId"` | ||
InvocationId string `json:"InvocationId"` | ||
InvokedOn time.Time `json:"InvokedOn"` | ||
} |
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.
I guess there is a copy of this structure somewhere in the vSwarm repo. Can you import it directly from there so we avoid duplicating structures and maintain consistency?
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, this is the same file, hut importing it from vSwarm directly had some issues with the go.mod file since this isn't directly declared as a package under the vSwarm repo
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.
I would give this another try?
type grpcVSwarmInvoker struct { | ||
cfg *config.LoaderConfiguration | ||
} | ||
|
||
func newGRPCVSwarmInvoker(cfg *config.LoaderConfiguration) *grpcVSwarmInvoker { | ||
return &grpcVSwarmInvoker{ | ||
cfg: cfg, | ||
} | ||
} |
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.
There is a lot of core repetition between this and the regular gRPC invoker. Consider extracting the common code and putting interface for the different part of the code. Same for the changes to pkg/driver/clients/invoker.go
.
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.
I had done that before, with both invokers essentially integrated into the same function switched by the flag, but @leokondrashov had asked me to create two separate files for two different invokers, or maybe I am misunderstanding something
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.
There is no contradiction. IIRC, I asked you to separate the two because you confusingly intermixed them. The proper way would be to make a general gRPC invoker call the actual gRPC with the correct protobuf (which you need to abstract away). Differences exist only in the Invoke function, which creates the protobuf and extracts the results. So, you need to make a gRPC client that uses the interface to make an actual call and extract the instance name, memory consumption, and actual duration.
I apologize if I caused confusion.
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.
Hi @cvetkovic @leokondrashov I have addressed your comments earlier and integrated the two invokers by extracting the common code and using an interface to execute the different RPCs. Please let me know if you have any other thoughts.
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.
cdd2a17
to
547100d
Compare
547100d
to
7e8e9e1
Compare
Signed-off-by: aryans1204 <arshar1204@gmail.com> Add VHIVE_REPO and LOADER_REPO to setup script config Signed-off-by: Mohsen Ghasemi <mohsenghasemi8156@gmail.com> Added tests and created new vSwarm invoker Fixed linting Fixed VSwarm Invoker for Knative mode Committer: aryans1204 arshar1204@gmail.com Fixed VSwarm Invoker for Knative mode Added new invoker for vSwarm functions and added tests Signed-off-by: aryans1204 <arshar1204@gmail.com> Fixed formatting under grpc_client.go Signed-off-by: aryans1204 <arshar1204@gmail.com> Fixed driver tests Signed-off-by: aryans1204 <arshar1204@gmail.com> Fixed linting and slimmed VSwarm tests Signed-off-by: aryans1204 <arshar1204@gmail.com> Fixed linting errors Signed-off-by: aryans1204 <arshar1204@gmail.com> Removed redundancies from Workload Signed-off-by: aryans1204 <arshar1204@gmail.com> Fixed vSwarm tests Signed-off-by: aryans1204 <arshar1204@gmail.com> Integrated vSwarm invoker into interface Signed-off-by: aryans1204 <arshar1204@gmail.com>
7e8e9e1
to
4733475
Compare
Summary
A small summary of the requirements (in one/two sentences).
Implementation Notes ⚒️
External Dependencies 🍀
Breaking API Changes⚠️
Simply specify none (N/A) if not applicable.