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

feat: Add side input sdkclient and grpc #953

Merged
merged 15 commits into from
Aug 25, 2023
Merged

Conversation

kohlisid
Copy link
Contributor

No description provided.

pkg/sideinputs/manager/manager.go Outdated Show resolved Hide resolved
pkg/sideinputs/manager/manager.go Outdated Show resolved Hide resolved
pkg/sideinputs/synchronizer/synchronizer.go Outdated Show resolved Hide resolved
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
@kohlisid kohlisid marked this pull request as ready for review August 22, 2023 19:41
@kohlisid kohlisid requested a review from vigith as a code owner August 22, 2023 19:41
@kohlisid kohlisid requested a review from jy4096 August 22, 2023 19:41
Signed-off-by: Sidhant Kohli <[email protected]>
@vigith
Copy link
Member

vigith commented Aug 22, 2023

tests are still failing

Signed-off-by: Sidhant Kohli <[email protected]>
pkg/sdkclient/sideinput/clienttest/clienttest.go Outdated Show resolved Hide resolved
pkg/sideinputs/manager/manager.go Show resolved Hide resolved
pkg/sideinputs/udsideinput/udsideinputs_grpc.go Outdated Show resolved Hide resolved
pkg/sideinputs/udsideinput/udsideinputs_grpc.go Outdated Show resolved Hide resolved
@kohlisid kohlisid requested a review from whynowy August 23, 2023 21:07

// UDSgRPCBasedUDSideinput applies user defined side input over gRPC (over Unix Domain Socket) client/server
// where server is the UDSideInput.
type UDSgRPCBasedUDSideinput struct {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add this proxy struct? The reason we have it in other places like udf is, they need to implement the MapApplier interface, it's not need to for side inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the client directly now

pkg/sideinputs/utils/utils.go Outdated Show resolved Hide resolved
pkg/sdkclient/sideinput/client.go Show resolved Hide resolved
Signed-off-by: Sidhant Kohli <[email protected]>
@kohlisid kohlisid requested a review from whynowy August 25, 2023 18:38
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Minor comments.

pkg/sideinputs/manager/manager.go Show resolved Hide resolved

// Check if the current value is same as the new value
// If true then don't update file again and return
if err == nil && bytes.Equal(currentValue, value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

in the future when file size gets larger we may consider using hash...

@vigith vigith merged commit f90d4fe into numaproj:main Aug 25, 2023
17 checks passed
@kohlisid kohlisid deleted the si-manager branch September 12, 2023 23:14
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.

5 participants