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

Tweaked invoker and added tests #540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aryans1204
Copy link

Summary

A small summary of the requirements (in one/two sentences).

Implementation Notes ⚒️

  • Briefly outline the overall technical solution. If necessary, identify talking points where the reviewer's attention should be drawn to.

External Dependencies 🍀

Breaking API Changes ⚠️

Simply specify none (N/A) if not applicable.

pkg/driver/clients/grpc_client.go Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
pkg/driver/clients/vhivemetadata.go Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
@leokondrashov
Copy link
Contributor

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.

@leokondrashov
Copy link
Contributor

Please squash your commits, sign off, and rebase the branch on current main.

@aryans1204 aryans1204 force-pushed the new-vswarm-invoker branch 2 times, most recently from 39cdf02 to 4c372fb Compare November 13, 2024 16:38
@aryans1204
Copy link
Author

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

Copy link
Contributor

@leokondrashov leokondrashov left a 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

server/trace-func-go/trace_func.go Outdated Show resolved Hide resolved
@aryans1204 aryans1204 force-pushed the new-vswarm-invoker branch 2 times, most recently from 197fbbc to af79127 Compare November 30, 2024 01:51
Copy link
Contributor

@leokondrashov leokondrashov left a 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.

pkg/driver/clients/grpc_client.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
server/trace-func-go/trace_func.go Outdated Show resolved Hide resolved
@aryans1204 aryans1204 force-pushed the new-vswarm-invoker branch 3 times, most recently from c447b94 to 46a4c3b Compare December 13, 2024 12:19
pkg/driver/clients/grpc_client.go Show resolved Hide resolved
pkg/workload/standard/workload.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_vswarm_client.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@leokondrashov leokondrashov left a 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

pkg/driver/clients/grpc_client_test.go Outdated Show resolved Hide resolved
pkg/driver/clients/grpc_vswarm_client.go Outdated Show resolved Hide resolved
Comment on lines +11 to +15
type vHiveMetadata struct {
WorkflowId string `json:"WorkflowId"`
InvocationId string `json:"InvocationId"`
InvokedOn time.Time `json:"InvokedOn"`
}
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Contributor

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?

Comment on lines 18 to 26
type grpcVSwarmInvoker struct {
cfg *config.LoaderConfiguration
}

func newGRPCVSwarmInvoker(cfg *config.LoaderConfiguration) *grpcVSwarmInvoker {
return &grpcVSwarmInvoker{
cfg: cfg,
}
}
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

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>
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.

3 participants