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

Add new firehose stream #35810

Merged
merged 1 commit into from Jul 20, 2020
Merged

Add new firehose stream #35810

merged 1 commit into from Jul 20, 2020

Conversation

daynew
Copy link
Member

@daynew daynew commented Jul 14, 2020

I'm implementing a new feature which will use Firehose but to a different Kinesis stream than the one we are already using. The PR just adds the ability to target different streams.

  • Update FirehoseClient.put_record to have a stream_name parameter so we can target different streams.
  • Update FirehoseClient so it has separate buffers for each Kinesis stream.

Links

Testing story

  • Unit tests

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@daynew daynew force-pushed the add_new_firehose_stream branch 5 times, most recently from 7c65ddc to 5f0c24a Compare July 16, 2020 02:12
@daynew daynew requested a review from wjordan July 16, 2020 18:59
@daynew daynew marked this pull request as ready for review July 16, 2020 18:59
Copy link
Contributor

@wjordan wjordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good! It could be merged as-is to unblock work moving forward. Added a couple of (optional) syntax/style comments below just for potential polish.

data_json: {
level_source_id: @level_source.id
}.to_json
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) to reduce the diff (and/or to remove some curly braces) you could use Ruby's implicit conversion of keyword parameters into a hash passed as the last argument:

FirehoseClient.instance.put_record(FirehoseClient::ANALYSIS_EVENTS_STREAM_NAME,
  study: 'share_filtering'
  [...etc...]
)

# string_key: 'home.heading_elementary_markdown'
# }
# )
I18N_STRING_TRACKING_EVENTS_STREAM_NAME = 'i18n-string-tracking-events'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names seem a bit verbose to me, I tend to prefer variables/constants passed around in business-logic code to be shorter/more readable based on context. How about something like a fixed hash to lookup the full stream name based on a short symbol, e.g.:

STREAMS = {
  analysis: 'analysis-events',
  i18n: 'i18n-string-tracking-events'
}

def put_record(stream, data)
  stream_name = STREAMS[stream] || raise "Stream #{stream} not found"
  [...]
end

FirehoseClient.instance.put_record(:i18n,
  url: 'http://studio.code.org/courses',
  string_key: 'home.heading_elementary_markdown'
)

wait_at_exit: 10.0
)
class Buffer < Cdo::Buffer
# Initializes the @firehose to an AWS Firehose client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment was already obsolete (and doubly so with this refactoring).

wait_at_exit: 10.0
)

raise ArgumentError.new("stream_name must be defined") if stream_name.nil? || stream_name.blank?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make a keyword argument required (and even better, caught at compile-time) by removing the default value, e.g.:

def initialize(stream_name:)

@daynew daynew merged commit c1a755f into staging Jul 20, 2020
@daynew daynew deleted the add_new_firehose_stream branch July 20, 2020 17:14
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.

None yet

2 participants