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.

Sign up for GitHub

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

Jump to bottom

Fixed non-working delay for oidc-client #14976

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Fixed non-working delay for oidc-client #14976

merged 1 commit into from
Feb 11, 2021

Conversation

steel-judoka
Copy link
Contributor

Fixes #14959

I also removed the wrong log info that stated "Connecting to IDP for up to %d times every 2 seconds" in fact according to the original idea, the code should have worked in such a way that after each unsuccessful request there should be a delay of 2 seconds.

@ghost
Copy link

ghost commented Feb 10, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

This message is automatically generated by a bot.

@steel-judoka steel-judoka changed the title Fixed issue #14959 Fixed non-working delay for oidc-client Feb 10, 2021
@sberyozkin
Copy link
Member

@Yelzhasdev thanks for the PR.
The way you collapsed all that manual iteration code into a backoff function looks neat.

But I'd like you to keep
"Connecting to IDP for up to %d times every 2 seconds" - it has been around for a while (in quarkus-oidc which is where this code has been copied from) and no-one has said it has been confusing - every 2 sec implies a delay of 2 seconds.
Please also consider applying the same code (once we settle on the final approach) to quarkus-oidc, see other comments as well
thanks

@sberyozkin sberyozkin added this to the 1.13 - master milestone Feb 10, 2021
@sberyozkin
Copy link
Member

@Yelzhasdev I just thought while I was offline that we should not touch quarkus-oidc for now - I'll take care of optimizing it later once I complete some other major refactoring there, hope you haven't started yet.

@steel-judoka
Copy link
Contributor Author

@Yelzhasdev I just thought while I was offline that we should not touch quarkus-oidc for now - I'll take care of optimizing it later once I complete some other major refactoring there, hope you haven't started yet.

@sberyozkin I didn't quite understand you, I was not going to touch quarkus-oidc, only quarkus-oidc-client in this pr

@sberyozkin
Copy link
Member

@Yelzhasdev I just thought, in my very first comment, that it makes sense to apply the same code to quarkus-oidc, but all is good, I'll take care of it separately

@sberyozkin
Copy link
Member

@Yelzhasdev Last step remains, please squash your commits, thanks

@steel-judoka
Copy link
Contributor Author

@sberyozkin Thanks! All done.

@sberyozkin sberyozkin self-requested a review February 11, 2021 10:18
@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 11, 2021
@sberyozkin sberyozkin merged commit 4aeba34 into quarkusio:master Feb 11, 2021
@gsmet gsmet modified the milestones: 1.13 - master, 1.12.0.Final Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

Assignees
No one assigned
Labels
triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.12.0.Final
Development

Successfully merging this pull request may close these issues.

OIDC Client does not use connection-delay property
3 participants
@steel-judoka @sberyozkin @gsmet