Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

RUBY-3429 Retry failed KMS requests #2907

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

comandeo-mongo
Copy link
Contributor

No description provided.

@comandeo-mongo comandeo-mongo requested a review from jamis November 18, 2024 09:16
Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

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

Looks good -- just the one question about integer division around the sleep call.

while (kms_context = Binding.ctx_next_kms_ctx(self)) do
begin
delay = Binding.kms_ctx_usleep(kms_context)
sleep(delay / 1_000_000) unless delay.nil?
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 either delay or 1_000_000 needs to be a float here, otherwise it will use integer division. Unless the intention here is to use integer division?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you! Fixed.

@comandeo-mongo comandeo-mongo requested a review from jamis November 19, 2024 11:29
Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

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

👍

@comandeo-mongo comandeo-mongo merged commit f1d1f91 into mongodb:master Nov 19, 2024
254 of 258 checks passed
@comandeo-mongo comandeo-mongo deleted the 3429-retry-kms branch November 19, 2024 15:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants