-
Notifications
You must be signed in to change notification settings - Fork 10
bail out of processing if fluentd stops + batch log stream processing… #12
Conversation
… (dont process all at once)
There seems to be a large number of unrelated formatting changes in this PR, which makes it quite difficult to understand what has changed functionally. Any chance you could make a cleaner changeset? |
For some reason circleci is not configured to build PRs. I'll look into this, but in the meantime I'd also appreciate if in addition to cleaning up the unrelated whitespace changes you could ensure the PR passes the linter: |
response = if !log_stream_name_prefix.empty? | ||
@aws.describe_log_streams( | ||
|
||
begin |
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.
There's no loop anymore, so the whole block has been left idented.
end | ||
rescue => boom |
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.
It will still retry in case of failures though
end | ||
rescue => boom | ||
prefix_message = !log_stream_name_prefix.empty? ? "with stream prefix #{log_stream_name_prefix}" : '' |
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.
changed the log message to include the log_stream_name_prefix only if it's set.
begin | ||
timestamp = state.store[group][stream]['timestamp'] | ||
timestamp = @event_start_time unless timestamp | ||
begin |
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.
this is a new "do-while" block, so things have been right idented.. see line 339
end | ||
end | ||
end while !@log_streams_next_token.nil? && !@finished # don't try to get and process all streams at once |
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.
save the next token for the log_streams globally... if we expose it out like this, we make sure we only process as many log_streams as they have been fetched from the method call
@@ -339,27 +348,31 @@ def run | |||
log.error("Unable to save state file: #{boom.inspect}") | |||
end | |||
|
|||
if event_count > 0 | |||
sleep_interval = @interval | |||
if !@finished # no need to sleep if it's finished |
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.
and if the finished is set, the program just needs to end right away
Sure, I'll cleanup the identation applied by default from RubyMine. There's one though from the main loop which is on purpose. I'll add my comments inline. |
@@ -197,35 +204,37 @@ def log_groups(log_group_prefix) | |||
|
|||
def log_streams(log_group_name, log_stream_name_prefix) | |||
log_streams = [] | |||
next_token = nil | |||
loop do |
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.
moved the loop to the "parent" function
6b64a51
to
ff0baf4
Compare
There are only two rubocop inspections left, which I haven't touched. |
Apologies it has taken so long to get this merged. At the minute I'm not in a good place to test this. Therefore I'm going to tag and publish it as To install pre-release versions of gems please see: http://guides.rubygems.org/patterns/#prerelease-gems |
What would help move the rc to a full release would be help in fixing some of the lint violations: https://circleci.com/gh/sampointer/fluent-plugin-cloudwatch-ingest |
Hey. It took some time to get the linter to pass this release. |
Thanks for that. I could only get to it today. Much appreciated. |
… (dont process all at once)
Sorry about the linting of RubyMine