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.

Sign up for GitHub

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

Jump to bottom

add separate server info file paths for services on client side #1494

Merged
merged 50 commits into from
Feb 21, 2024
Merged

add separate server info file paths for services on client side #1494

merged 50 commits into from
Feb 21, 2024

Conversation

ayildirim21
Copy link
Member

Implement #1456 on client side

@ayildirim21 ayildirim21 marked this pull request as draft February 8, 2024 02:14
@KeranYang
Copy link
Member

@a3hadi , with this change, the platform stops supporting all the old UDF images (hence all e2e fail.). Users have to re-build UDF/Source/Sink images using the latest SDKs. Since server info hasn't been widely used, let's see if we can update on the platform side in a way that when a server info file is not found because of incompatible file path, the platform numa container can decide to ignore it and continue processing.

@KeranYang
Copy link
Member

@a3hadi , could you also look into why the udsource e2e succeeded? Thanks!

@ayildirim21
Copy link
Member Author

@a3hadi , with this change, the platform stops supporting all the old UDF images (hence all e2e fail.). Users have to re-build UDF/Source/Sink images using the latest SDKs. Since server info hasn't been widely used, let's see if we can update on the platform side in a way that when a server info file is not found because of incompatible file path, the platform numa container can decide to ignore it and continue processing.

I think ignoring incompatible server info path, with a message to remind to upgrade sdk is a good idea (since using the new server info paths isn't mission critical to continue processing anyways). I will focus on getting the current tests to pass first and then look into this.

@ayildirim21
Copy link
Member Author

After discussing with Keran, I think the best course of action for the "error handling" is simply, if the server info file path is not found, set it to the default path of /var/run/numaflow/server-info. This is because, the server info file will always be created. However there are two possible paths that it can have. If an old sdk version is being used it will be the aforementioned default path, if the sdk version is up to date it will be using the new isolated paths for each service. Thus falling back to the default path when the server info file path is not found will accommodate older sdk versions.

@ayildirim21 ayildirim21 marked this pull request as ready for review February 14, 2024 15:49
@ayildirim21 ayildirim21 marked this pull request as draft February 14, 2024 15:54
ayildirim21 and others added 12 commits February 15, 2024 11:45
@ayildirim21
update all images for reduce-e2e
4e97cd4 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update images for reduce-sdk-e2e
7ecbc7a 
Signed-off-by: a3hadi <[email protected]>
@KeranYang
enable udsource info file, break current e2e
fa3a3e3 
Signed-off-by: Keran Yang <[email protected]>
@ayildirim21
modify reduce-stream-java
1870909 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
Merge branch 'serverinfo-path-1456' of github.com:a3hadi/numaflow int…
1e335dd 
…o serverinfo-path-1456
@ayildirim21
modify reduce-stream-java
a5a27b3 
Signed-off-by: a3hadi <[email protected]>
@KeranYang
test with new image
8ce1116 
Signed-off-by: Keran Yang <[email protected]>
@KeranYang
fix: make udsource e2e a real test
2561d4a 
It was a weak e2e test because any logs can contain string "0", "1" and "2".

Signed-off-by: Keran Yang <[email protected]>
@KeranYang
chore: update udsource sdkclient directory to be consistent with others
086f58e 
Signed-off-by: Keran Yang <[email protected]>
@ayildirim21
Merge branch 'main' into serverinfo-path-1456
ab400af
@ayildirim21
update all images in sdks-e2e
086890f 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
Merge branch 'serverinfo-path-1456' of github.com:a3hadi/numaflow int…
fc20aac 
…o serverinfo-path-1456
ayildirim21 and others added 15 commits February 18, 2024 23:57
@ayildirim21
test
e8af7fe 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update go.mod
f8195b5 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update all images for sideinputs-e2e
bac6b62 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update image in transformer-e2e
22c85bd 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update images for udsource-e2e
cece0d6 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update image in nats-e2e
c8a0ec8 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update image in idle-source-e2e
533db76 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update images in http-e2e
3a55587 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update images in e2e
b5da320 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
update images in diamond-e2e
77725dc 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
modify ud-source test
d3d7d4b 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
modify ud-source test
7677ac0 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21
Merge branch 'main' into serverinfo-path-1456
81c883d
@KeranYang
fix lint
736409d 
Signed-off-by: Keran Yang <[email protected]>
@ayildirim21
update reduce-sideinput-pipeline.yaml
4096121 
Signed-off-by: a3hadi <[email protected]>
@ayildirim21 ayildirim21 marked this pull request as ready for review February 20, 2024 16:24
KeranYang
KeranYang reviewed Feb 20, 2024
View reviewed changes
pkg/sdkclient/mapper/client.go Outdated Show resolved Hide resolved
pkg/sdkclient/options.go Outdated Show resolved Hide resolved
test/diamond-e2e/testdata/join-on-reduce-pipeline.yaml Outdated Show resolved Hide resolved
KeranYang
KeranYang approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@KeranYang KeranYang left a comment

Choose a reason for hiding this comment

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

LGTM. Please add back the interface check and then we can merge this in.

pkg/sdkclient/sinker/client.go Show resolved Hide resolved
pkg/sdkclient/source/client.go Show resolved Hide resolved
@KeranYang KeranYang merged commit 37a9d5d into numaproj:main Feb 21, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KeranYang KeranYang KeranYang approved these changes

@whynowy whynowy Awaiting requested review from whynowy whynowy is a code owner

@vigith vigith Awaiting requested review from vigith vigith is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ayildirim21 @KeranYang