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

Use futures in the Source poll call and logging enhancements #28

Closed
wants to merge 1 commit into from
Closed

Use futures in the Source poll call and logging enhancements #28

wants to merge 1 commit into from

Conversation

drnushooz
Copy link

@drnushooz drnushooz commented Mar 29, 2022

The poll call in the source task today uses a single thread. From our internal tests, it has showed better performance when CompletableFutures are used at it distributes the load among multiple threads. In addition, there is also additional debug level logging to show which Redis node the data is coming from to debug any issues pertinent to data distribution in Redis. This logging has helped us internally to diagnose some problems. The PR also makes maxPollSize a configurable parameter with the same default.

@drnushooz drnushooz changed the title Use futures in the Source poll call and logging enhancements. Use futures in the Source poll call and logging enhancements Mar 29, 2022
@drnushooz drnushooz closed this Mar 29, 2022
@jaredpetersen
Copy link
Owner

Talked through a separate side channel -- I'm not convinced that this will solve the performance issues since the bottleneck doesn't appear to be related to transforming data and putting it in a queue. Basically, we need a way to better parallelize the connector so that keyspace notifications parallelize based on the node. The solution for that is dependent on a fix for #27.

I'm also not convinced at a glance that this PR isn't dropping records once max poll is reached.

More testing in this area is needed and I'm currently pre-occupied with other projects and personal stuff during my evening hours. I'm actively interested in improving the performance however and I'll file a new ticket for this.

@jaredpetersen
Copy link
Owner

Filed #29

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.

2 participants