Skip to content

feat: implement sdk for session reduce#94

Merged
yhl25 merged 26 commits into
numaproj:mainfrom
KeranYang:session
Mar 13, 2024
Merged

feat: implement sdk for session reduce#94
yhl25 merged 26 commits into
numaproj:mainfrom
KeranYang:session

Conversation

@KeranYang
Copy link
Copy Markdown
Member

@KeranYang KeranYang commented Jan 29, 2024

Tested using the existing golang sdk e2e: KeranYang/numaflow#98

Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
@KeranYang KeranYang marked this pull request as ready for review January 29, 2024 01:15
@KeranYang KeranYang requested review from a team and yhl25 January 29, 2024 01:15
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
@whynowy whynowy marked this pull request as draft February 5, 2024 18:21
@whynowy
Copy link
Copy Markdown
Member

whynowy commented Feb 5, 2024

Convert to draft mode since it's taking too long to review.

Copy link
Copy Markdown
Contributor

@yhl25 yhl25 left a comment

Choose a reason for hiding this comment

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

partial review

Comment thread src/main/java/io/numaproj/numaflow/sessionreducer/SessionReducerActor.java Outdated
Comment thread src/main/java/io/numaproj/numaflow/sessionreducer/SessionReducerActor.java Outdated
Comment thread src/main/java/io/numaproj/numaflow/sessionreducer/SessionReducerActor.java Outdated
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
.
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
@KeranYang KeranYang requested a review from yhl25 March 12, 2024 14:03
@KeranYang KeranYang marked this pull request as ready for review March 12, 2024 14:03
@KeranYang KeranYang requested a review from a team March 12, 2024 14:03
Comment on lines +90 to +92
// set time out to 1 second as we expect a MERGE operation to finish quickly.
Timeout timeout = new Timeout(Duration.create(1, "seconds"));
try {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hacky, sometimes it might take more than 1 second

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks, what should be the proper number? @yhl25

Copy link
Copy Markdown
Contributor

@yhl25 yhl25 Mar 12, 2024

Choose a reason for hiding this comment

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

let's wait until it returns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also, please add unit tests to cover merge followed by another operation to make sure the blocking call works!

Copy link
Copy Markdown
Member Author

@KeranYang KeranYang Mar 12, 2024

Choose a reason for hiding this comment

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

The method Await asks for a timeout, I don't think we should wait forever, right? Maybe we should set an atMost timeout value and throws when the operation takes too long.

Yes, the uts for merge are already in place. Please check the serverTest class.

Signed-off-by: Keran Yang <yangkr920208@gmail.com>
@yhl25 yhl25 merged commit 68ffc5a into numaproj:main Mar 13, 2024
KeranYang added a commit to KeranYang/numaflow-java that referenced this pull request Jan 22, 2025
Signed-off-by: Keran Yang <yangkr920208@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