-
-
Notifications
You must be signed in to change notification settings - Fork 353
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
Support kinesis #143
Support kinesis #143
Conversation
@region = options[:region] | ||
@host = options[:host] || "kinesis.#{options[:region]}.amazonaws.com" | ||
@path = options[:path] || '/' | ||
@persistent = options[:persistent] || false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false is likely the right default, but in certain cases we might be able to coax performance with persistent enabled. Different AWS services seem to tolerate/support this to different extents, so hard to say for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I actually ran my test app with persistent: true
without any issues, so maybe it is worth changing the default here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. It's almost always bit me when I set it to true. It just depends on the usage pattern though. I think, with S3 for instance, persistent works great as long as you don't let it timeout (ie be active at least once per 30 seconds). But with a bit of quiet it dies/explodes. Anyway, something to consider, because the dns/connect/ssl stuff adds up pretty quickly.
Looking good, a few comments above to consider. Just let me know if you have other questions or anything I can help with. |
@@ -219,7 +221,7 @@ def self.parse_security_group_options(group_name, options) | |||
|
|||
def self.json_response?(response) | |||
return false unless response && response.headers | |||
response.get_header('Content-Type') =~ %r{application/json}i ? true : false | |||
response.get_header('Content-Type') =~ %r{application/.*json.*}i ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geemus kinesis has a new content type: x-amz-json-1.1
. Actually it looks like data_pipeline and dynamodb use a similar content type, but apparently this is maybe the first time json_response?
was need to support error parsing. Does this new match look ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably fine.
@geemus I think this is ready to go with the possible exception of adding more non-happy path tests, but maybe this is good enough for a start. I've been able to run with and without mocking consistently. |
Thanks! |
TODO: