-
Notifications
You must be signed in to change notification settings - Fork 32
Advanced mode uses individual configs properly. #104
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
Conversation
| options.setMaxBatchSize(max_batch_size) | ||
| options.setPrefetchCount(prefetch_count) | ||
| options.setReceiveTimeOut(Duration.ofSeconds(receive_timeout)) | ||
| options.setMaxBatchSize(event_hub['max_batch_size']) |
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.
review note: global settings will be set if no individual settings exist in the register method - https://github.com/logstash-plugins/logstash-input-azure_event_hubs/pull/104/files#diff-2f52a24e41e999858b41f58fe5365f020d090a206216dadf30774c5a8385647eL387
| } | ||
| end | ||
| it_behaves_like "an exploded Event Hub config", 1 | ||
| it "it explodes the 2cnd advanced config event hub correctly" 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.
Can you explain why there are 3 sets of event hubs defined here, but only 2 verified?
I think this is leading to missing testing some of the override settings, as the tests are done against the "no overriden settings" event hub and the "partially overriden settings" event hub - eg prefetch_count and initial_position_lookback only ever test the top-level setting, and not an individual event hub override.
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.
Can you explain why there are 3 sets of event hubs defined here, but only 2 verified?
First entry (index is 0) is verified in it_behaves_like "an exploded Event Hub config", 1
prefetch_count (30) and initial_position_look_back (50) are different in first entry. 2nd and 3rd entries are using global 'prefetch_count' => 250 and 'initial_position_look_back' => 7200 settings.
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.
Ah, thank you. Missed that completely
donoghuc
left a comment
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 looks correct to me.
robbavey
left a comment
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.
lgtm
Release notes
Fixes the bug where with
config_mode => 'advanced', event hub-specific settings (initial_position,max_batch_size,prefetch_count,receive_timeout,initial_position_look_back) were being ignored and replaced with global defaults. These settings are now correctly applied per event hubWhat does this PR do?
When using
config_mode => 'advanced'and hub-specific settings (initial_position,max_batch_size,prefetch_count,receive_timeout,initial_position_look_back) in eachevent_hubsentry, those settings were ignored and replaced with global defaults.This PR correctly applies settings per event hub considering the global params.
Why is it important/What is the impact to the user?
User who use
config_mode => 'advanced'andevent_hubsentry settings such asinitial_position,max_batch_size,prefetch_count,receive_timeoutandinitial_position_look_back, they do not experience expected behaviour that individual settings will not be applied, instead they are replaced by global settings.Checklist
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs