Skip to content

Add --num-threads option to query-cloudwatch#9444

Merged
zachmargolis merged 1 commit intomainfrom
margolis-query-cloudwatch-num-threads
Oct 25, 2023
Merged

Add --num-threads option to query-cloudwatch#9444
zachmargolis merged 1 commit intomainfrom
margolis-query-cloudwatch-num-threads

Conversation

@zachmargolis
Copy link
Contributor

Leftover from pairing on LG-11381

**Why**: Option to increase parallelism

changelog: Internal, Scripts, Add --num-threads option to query-cloudwatch
@zachmargolis zachmargolis requested a review from a team October 25, 2023 00:23
'--num-threads NUM',
"number of threads, defaults to #{Reporting::CloudwatchClient::DEFAULT_NUM_THREADS}",
) do |str|
config.num_threads = Integer(str, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking out of curiosity: is there a reason we explicitly specify that this is base 10 (the second argument)? I see mixed use in the project. AFAICT the default if not specified is base 10; any other behavior would feel very surprising.

I have no objection at all to including it; I'm just curious if there's a fun story here or if it's just a pattern we use on the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, fun, disregard! I answered my own question with an example of my childhood ZIP code:

[12] pry(main)> Integer("03054")
=> 1580
[13] pry(main)> Integer("03054", 10)
=> 3054

This interpretation was one of my first surprises years ago when I first started working with Ruby. (Also also a good lesson in ZIP codes being strings, not integers.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Integer() method returns wrong results with leading zeroes, so this is about strictness. And I chose Integer over .to_i because .to_i but that silently returns 0 with non-numeric things, but Integer will raise

Integer('077') # unexpected value, bad
=> 63
Integer('077', 10) # right value, good
=> 77
> Integer('a', 10) # should error, good
(irb):6:in `Integer': invalid value for Integer(): "a" (ArgumentError)
> "a".to_i # won't error, bad
=> 0

@zachmargolis zachmargolis merged commit c577c87 into main Oct 25, 2023
@zachmargolis zachmargolis deleted the margolis-query-cloudwatch-num-threads branch October 25, 2023 16:42
@mdiarra3 mdiarra3 mentioned this pull request Oct 26, 2023
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.

2 participants