-
Notifications
You must be signed in to change notification settings - Fork 164
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
KinesisMessageDrivenChannelAdapter consumes nearly all CPU time #70
Comments
Well, that is normal behavior of the infinite loop, like in the
And it really eats the Thread and, therefore, one of the CPU cores. Do you want to have some sleep in between cycles and let other resources to use the CPU? Hope I am clear. |
Especially in the cloud environment (e.g AWS) I would like to distinguish situations either the server is running under load or not. And don't want to get CloudWatch alarms about 100% CPU consumption constantly. |
@artembilan Sorry. It is not clear for me. It would be nice if you could give me more details? |
Well, my idea that we dedicate a Thread (or the whole Thread Pool) to our application and don't care how it manages them. As I said the code looks like an infinite loop without any timeouts in between cycles, especially with the If you have some ideas how to resolve that I'm open for suggestions. Like My current point like I don't want to add sleeps into those loops because we will have some lag between consumer cycles. @garyrussell , @mbogoevici , WDYT about this concern? |
If there is no blocking API, I think there is no alternative but to add a (configurable) I am not familiar with the code, but the sleep can be made conditional on no data being received. |
Well, I went ahead and decided to fix this any way: a8d370f. Thank you @garyrussell for advice. And special thanks to @volodarsky for catching this problem. My apologies not being so thoughtful from the beginning. Would be great if you are able to test against the latest |
I can't read the code on my phone but think the sleep must be configurable and 1ms is not a suitable default. Maybe 100ms would be better. We should only sleep if no data is received. |
Well, The problem that we don't know in that place that we should sleep or not. It is a dispatcher thread. The real task is done by the target But I'm open for suggestions. I'm almost agreed with my self that there should be some But yeah... That's IMO and any recommendations are welcome |
I understand, but cloud billing is usually based on compute time, so spinning a CPU to do no work is just terribly awful. I am not familiar with the code so I don't have a recommendation; sorry. |
@artembilan very thanks for your quick fix, however I'm observing that CPU consumption stay nearly on the same level. Even then I've re-build snapshot with 100 ms sleep delay, I discovered the same behaviour. (I've attached some screenshot - hope it could helps)
|
Btw, then I've added sleep delay in the main cycle and situation has improved significantly: |
Thank you, @volodarsky , for testing and feedback. Well, since it doesn't help I'll think more about some barrier feature. Just let me know: do we how now problem only with the |
H-m. Funny.
So, they sleep after each consuming cycle. And that is like:
by default... |
Well, OK. I see this: http://docs.aws.amazon.com/streams/latest/dev/kinesis-low-latency.html
And then in the end:
So, I intend to go ahead with the How is this plan for you? |
Yep.
Seems it will be ok.
25 records limit are definitely too small, but I'm not sure about 10k - as I remember, kinesis record size could be up to 2mb. (remember about billing of number of records in AWS) I'd rather suggest 100 - 500 |
Hm, I can't find where I picked up
And this is a default for Kinesis Client Library. Looks like the reply is restricted to the 10MB anyway:
http://docs.aws.amazon.com/cli/latest/reference/kinesis/get-records.html What you talk about the price is a number of records in the stream, not how many you would like to poll. |
My point was that AWS customers will tend to use big zipped records to reduce their costs. And this could leads to unexpected memory load. But with 10 mb constraint it doesn't make sense. |
OK! I added Let me know how does it work now for you. Thank you very much for all the feedback! |
@artembilan Thanks a lot! Now it works fine. Btw, when could I expect this fix will be appeared as a milestone/release in maven repo? |
That's great! Well, look, having the started work for the SCSt Kinesis Binder (https://github.com/spring-cloud/spring-cloud-stream-binder-aws-kinesis), we realised that there are some missed options and features to be implemented here. OTOH I understand your expectation and won't mind to release M2 for this critical bug. We are releasing Spring Integration 4.3.10 this week. So, this one will be upgraded and released after that. |
The version Thank you for your feedback one more time! |
I've encountered that KinesisMessageDrivenChannelAdapter's ConsumerDispatcher & ConsumerInvoker consumes nearly all CPU even without any load. I used 'KinesisMessageDrivenChannelAdapter' with default settings. What could I do to decrease CPU load?
The text was updated successfully, but these errors were encountered: