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

Addition of the inbound part of Redis Stream support. #3350

Closed
wants to merge 3 commits into from

Conversation

akuma8
Copy link
Contributor

@akuma8 akuma8 commented Jul 28, 2020

#3226
This commit is for a first review since tests are not yet added.

@akuma8
Copy link
Contributor Author

akuma8 commented Jul 28, 2020

@artembilan,
This is the inbound PR. I am working on adding tests while you are reviewing.
Thanks

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Some review while you're writing tests...

@akuma8

This comment has been minimized.

@artembilan
Copy link
Member

You should have a ReactiveRedisStreamMessageHandler as a bean. This not a ReactiveMessageHandlerAdapter responsibility to initialize its delegate.

@akuma8
Copy link
Contributor Author

akuma8 commented Aug 3, 2020

Hi @artembilan,
I completed outbound side tests by adding the test with the List class.
I have an issue with inbound tests where one of them turn into an infinite loop.
I am not enough familiar with the Reactive world, which explains my problems.
Could you take a look and let me know?
Thanks a lot

@akuma8
Copy link
Contributor Author

akuma8 commented Aug 5, 2020

Hi @artembilan,
I did requested changes.
I still have the same issue with my tests.
I think there is an issue with the configuration of streamReceiver.
I breakpointed to check the content of messageFlux (result of the this.streamReceiver.receive()), I can see a StackOverflow exception, see attached image.
StackOverflow
I don't kknow what I am doing wrong since I use a default streamReceiver:

StreamReceiver.StreamReceiverOptions<String, ?> streamReceiverOptions = StreamReceiver.StreamReceiverOptions.builder().pollTimeout(Duration.ZERO).build();
this.streamReceiver = StreamReceiver.create(this.reactiveConnectionFactory, this.streamReceiverOptions);

I think there is an issue with the receive() method.

akuma8 and others added 3 commits August 5, 2020 14:49
This commit is for a first review since tests are not yet added.
…g the inbound side, one test turns into an infinite loop and one is not yet implemented. We have to first find the reason of the infinite loop before continuing.

Toso
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Pulling locally for review and possible merge after cleaning up...

@artembilan
Copy link
Member

Merged as 4ae6b52.

Please, pull the latest master locally to investigate what I did for the code.

I'm really looking forward for more tests since we have fixed the problem with connection factory.
Plus it would be great to have some docs in the redis.adoc on the matter.

Thank you for contribution!

@artembilan artembilan closed this Aug 7, 2020
@artembilan artembilan added this to the 5.4 M2 milestone Aug 7, 2020
@akuma8
Copy link
Contributor Author

akuma8 commented Aug 10, 2020

@artembilan thanks for your investigation.
Did the problem come from the connection factory?
I am working on adding further tests.
About documentation, as I don't speak english it will be hard for me to write a good documentation.
I will try and let you know.
Thank you

@artembilan
Copy link
Member

Hi @akuma8 !

Yes, I think the problem for hanging test suite was really about the second call for the ConnectionFactory.afterPropertiesSet() where we created uncontrolled RedisClient.

Sure! More tests are welcome!
Re. Docs: I'm not good in English, too: @garyrussell is always helping me to make my docs PRs better.
So, feel free to raise whatever you can do best and we together will bring it into the public!

Thank you!

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.

None yet

2 participants