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

Add Retry logic in createOrReplace when 500 received from ApiServer: Workaround for #2292 #2501

Merged
merged 2 commits into from
Nov 10, 2020
Merged

Add Retry logic in createOrReplace when 500 received from ApiServer: Workaround for #2292 #2501

merged 2 commits into from
Nov 10, 2020

Conversation

FWiesner
Copy link
Contributor

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@centos-ci
Copy link

Can one of the admins verify this patch?

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_265) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@manusa
Copy link
Member

manusa commented Sep 21, 2020

Similar methods can also be found in:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java

Line 134 in aa651fa

public HasMetadata createOrReplace() {

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java

Line 256 in c65b454

public List<HasMetadata> createOrReplace() {

The issue should be tackled there too.

Maybe we can extract the logic to some helper or utils class (maybe in the scope of another issue > future).

@rohanKanojia
Copy link
Member

@FWiesner : Are you still working on this? I understand you might be busy with your release. If so, we can pick this up and get this merged before the upcoming release.

@FWiesner
Copy link
Contributor Author

FWiesner commented Oct 9, 2020

Hi @rohanKanojia it would be great if you could pick it up - release is over, but I'm still under water and ill on top

@rohanKanojia rohanKanojia force-pushed the issue/2292 branch 3 times, most recently from a1507ae to 8feda92 Compare October 20, 2020 20:41
@rohanKanojia rohanKanojia marked this pull request as ready for review October 21, 2020 06:40
@rohanKanojia rohanKanojia force-pushed the issue/2292 branch 2 times, most recently from 51bd2dc to 57838a0 Compare October 27, 2020 10:02
@manusa manusa self-requested a review November 9, 2020 10:52
manusa
manusa reviewed Nov 9, 2020
View reviewed changes
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java Outdated
@@ -471,4 +473,11 @@ public static String getSystemPathVariable() {
private static String getOperatingSystemFromSystemProperty() {
return System.getProperty(OS_NAME);
}

public static boolean isHttpStatusCodeFromErrorEncounteredByServer(int code) {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not checking if the HTTP status code belongs to a server error family (code >= 500), and for our use-case we just want to retry if one of these status codes is returned by the server, it's probably a good idea to move the method to CreateOrReplaceHelper class.
Maybe also rename it to shouldRetry or something like that, which suggests that the procedure is only retried for some specific status codes.

@rohanKanojia rohanKanojia force-pushed the issue/2292 branch 2 times, most recently from 310c87c to cfd4be1 Compare November 9, 2020 16:44
@rohanKanojia
Changes in BaseOperation to retry createOrReplace on server failure
2aaa79b 
+ Moved createOrReplace logic to CreateOrReplaceHelper class
+ Added tests
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

57.3% 57.3% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_272) you have used to run this analysis is deprecated and we will stop accepting it accepting it soon.Please update to at least Java 11.
Read more here

@manusa manusa added this to the 4.13.0 milestone Nov 10, 2020
@manusa
Copy link
Member

manusa commented Nov 10, 2020

[merge]

@FWiesner
Copy link
Contributor Author

awesome - thanks!

@rohanKanojia
Copy link
Member

[merge]

@fusesource-ci fusesource-ci merged commit 67b8bb8 into fabric8io:master Nov 10, 2020
@FWiesner FWiesner deleted the issue/2292 branch November 10, 2020 12:00
@rohanKanojia rohanKanojia changed the title WIP: Workaround for #2292 Add Retry logic in createOrReplace when 500 received from ApiServer: Workaround for #2292 Nov 10, 2020
@fusesource-ci
Copy link
Contributor

Pull request is not mergeable.

@rohanKanojia rohanKanojia mentioned this pull request Nov 17, 2020
Closed
@manusa manusa mentioned this pull request Apr 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanKanojia rohanKanojia rohanKanojia left review comments

@manusa manusa manusa approved these changes

Assignees

@rohanKanojia rohanKanojia

Labels
None yet
Projects
None yet
Milestone
4.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@FWiesner @centos-ci @manusa @rohanKanojia @fusesource-ci