-
Notifications
You must be signed in to change notification settings - Fork 3
Initial Commit #2
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
base: main
Are you sure you want to change the base?
Conversation
Yay! welcome back and thanks for your contribution, I will start again the reviews in here 😺 keep talking! |
module Logstash | ||
module Inputs | ||
module DynamoDB | ||
class LogStashRecordProcessor |
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.
As we used the new ruby based namespace model here, the logstash part of the class name Is a bit redundant, isn't?
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.
The implementation of these classes follows a convention that has been set when using KCL. I believe even if it's redundant, it's better to keep a consistent experience across our KCL applications.
@raymolin I've execute
but it gets hanged by the end of the all execution, does this happen to you? might be a thread is waiting to be closed? multithreading with rspec can be super tricky. |
Will need to investigate the testing a little bit more... taken care of rest of comments Update 08/25 - Doesn't seem to be a multi threading issue (dumped all threads at the end of each test), but have isolated the problem to the dynamodb.rb file (doesn't hang if this test isn't run). Not too familiar with the internals of this file, so I'll go ahead and look into it some more Another update, the issue seems to stem from allow_invalid_credentials method in spec/inputs/dynamodb_spec.rb... Looks like it has something to do with when we register a new plugin... will continue to investigate. |
@purbon There seems to be an issue using mocha to mock explicit Java Objects. Do you have any thoughts on this? At this point, this seems to be a framework limitation that is blocking the release, even though the test themselves pass. Perhaps we could disable these tests by default and make a note discussing these limitations. |
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'. |
@raymolin following up on this. did you ever get to signing our CLA? |
👍 |
Was this merged in another PR? Did progress here stop? |
Package was handed off from Brandon Tom after the edits to code review from Elastic were made.