Fix for issue #413: Circuit Breaker Pattern by PalAditya · Pull Request #986 · iluwatar/java-design-patterns

PalAditya · 2019-10-12T14:37:04Z

Fixes issue #413 by adding an implementation of circuit breaker pattern

Added the Circuit Breaker Pattern
Wrote test cases with nearly 100% coverage for all of the component classes (except the main class)

An example output:

Code Coverage Report:

Please mention whether something else needs to be done for this PR 😄

iluwatar

There is an error related to generating Javadocs in Travis build, although the check looks green. See the following screenshot

PalAditya · 2019-10-13T07:39:48Z

There is an error related to generating Javadocs in Travis build, although the check looks green. See the following screenshot

I see. I did build it locally but I use Java 8 and didn't run up against this error, and I didn't go through the Travis CI logs (😅 ). Any idea on why this could be happening?

It does seem like this is a common problem on Java 11+ builds though, as written here, and I'll try to fix it as such.

Do you have any other changes in mind?

PalAditya · 2019-10-13T07:44:16Z

Actually, I see that many modules don't seem to use the javadocs plugin, and it's not in the parent pom, so is it necessary or should I remove that from the pom.xml of circuit-breaker all together?

iluwatar · 2019-10-13T08:06:55Z

@PalAditya yes using that plugin is not mandatory so it is ok to remove

PalAditya · 2019-10-13T09:33:36Z

I believe that the previous error stemmed from this part only, and as you can see, I have fixed that, but I am getting a JaCoCo error from a completely unrelated module. Any advice on that? 😅

Note: Could be related to this or this

iluwatar · 2019-10-13T20:18:12Z

I believe the jacoco issue was fixed in #996. Please merge master into this branch.

iluwatar · 2019-10-13T20:21:53Z

+					</execution>
+				</executions>
+			</plugin>
+    </plugins>


The plugins are already defined in parent pom.xml and not needed here

iluwatar · 2019-10-13T20:24:58Z

+    MonitoringService obj = new MonitoringService();
+    //Set the circuit Breaker parameters
+    CircuitBreaker circuitBreaker = new CircuitBreaker(3000, 1, 2000 * 1000 * 1000); 
+    long serverStartTime = System.nanoTime();


You can use local-variable type inference here (var)

iluwatar · 2019-10-13T20:27:55Z

+  long timeout;
+  long retryTimePeriod;
+  long lastFailureTime;
+  int failureCount;
+  private final int failureThreshold;
+  private State state;


Use final where member variables don't change

iluwatar · 2019-10-13T20:31:33Z

+        DelayedService delayedService = new DelayedService(20);
+        String response = delayedService.response(serverStartTime);


Use local-variable type inference (var)

iluwatar · 2019-10-13T20:32:00Z

+    this.timeout = timeout;
+    this.retryTimePeriod = retryTimePeriod;
+    //An absurd amount of time in future which basically indicates the last failure never happened
+    this.lastFailureTime = System.nanoTime() + 1000 * 1000 * 1000 * 1000;


Could it be constant? Looks a bit cryptic

iluwatar · 2019-10-13T20:33:50Z

+   */
+  public String response(long serverStartTime) {
+    long currentTime = System.nanoTime();
+    if ((currentTime - serverStartTime) * 1.0 / (1000 * 1000 * 1000) < delay) {


The calculation here is hard to understand. We need at least comment there.

iluwatar · 2019-10-13T20:35:27Z

+    //Set time as current time as initially server fails
+    long serverStartTime = System.nanoTime();
+    String response = monitoringService.remoteResourceResponse(circuitBreaker, serverStartTime);
+    assertEquals(response, "Remote service not responding");


Also in tests var can be used extensively

PalAditya · 2019-10-13T21:44:14Z

Thanks for the feedback! I implemented the changes as requested, but the Sonar Integration seems to have broken something else.

A local build with mvn clean install still passes perfectly though, so I believe the error is not with the module but somewhere else, which is causing the CI to fail. Correct me if I'm wrong 🤔

iluwatar · 2019-10-14T05:48:19Z

I wonder why the sonar analysis worked in my branch and master, but not here. To remedy this, I now tried to set the sonar host property in 47d92bb Could you merge from the master again so we can see if it helped?

PalAditya · 2019-10-14T06:34:53Z

After merging, I get this. It seems like it's an permission issue on your end 😅
Maybe it is enabled for master but not PRs?
Also, this might help.

PalAditya · 2019-10-14T07:52:20Z

Funny thing: I decided to play around with the config file and made a temporary copy of the project on my SonarQube account, and guess what, it passed (with this minimal yml on another branch)!

language: java
dist: bionic
jdk:
- openjdk11
sudo: required

services:
- xvfb

addons:
  sonarcloud:
    organization: "paladitya"
    
script:
  # the following command line builds the project, runs the tests with coverage and then execute the SonarCloud analysis
  - mvn clean verify sonar:sonar -Dsonar.projectKey=PalAditya_java-design-patterns -Dsonar.host.url=https://sonarcloud.io

notifications:
  email:
    - adityapal.nghss@gmail.com
  webhooks:
    on_success: change  # options: [always|never|change] default: always
    on_failure: always  # options: [always|never|change] default: always
    on_start: never     # options: [always|never|change] default: always

I can only guess that either the encrypted key is not being read properly for PRs (I actually set the value SONAR_TOKEN in my travis settings rather than encrypting it), or maybe the execute-analysis permission needs to be set for everyone?

(Sorry if my guesses are unhelpful, but I have never worked with SonarQube before, and I find this prospect of a bug/misconfiguration quite interesting 😅 )

iluwatar · 2019-10-14T16:14:55Z

https://docs.travis-ci.com/user/sonarcloud/#analysis-of-internal-pull-requests
The documentation indicates that it is not supposed to work across forks

PalAditya · 2019-10-14T16:21:45Z

https://docs.travis-ci.com/user/sonarcloud/#analysis-of-internal-pull-requests
The documentation indicates that it is not supposed to work across forks

I see. So, what should be done next from my end? 🤔

iluwatar

The code in README.md explanation is not in sync with the implementation

PalAditya · 2019-10-14T16:25:05Z

The code in README.md explanation is not in sync with the implementation

Oh, do you mean that I should change them to use var and final too or that I shouldn't remove the comments that are in the actual code?

I am going to assume the former since I don't see a heavy usage of comments in the other READMEs and I have written the explanation too. Plus, they could always see the actual code for full understanding.

iluwatar · 2019-10-14T17:26:52Z

@PalAditya can you please merge master again? It shouldn't run the sonar analysis for this PR now, if everything goes fine.

PalAditya · 2019-10-14T18:10:00Z

@iluwatar This seems to have done the trick 😄

iluwatar · 2019-10-14T18:25:09Z

Wonderful, thanks for your help @PalAditya 👍

iluwatar · 2019-10-14T18:32:42Z

Thanks for the great pattern @PalAditya, well done

PalAditya added 2 commits October 12, 2019 19:57

Fix Issue #413: Circuit-Breaker Pattern

1cb1bc5

Fix Image Links

f7a821a

iluwatar self-assigned this Oct 13, 2019

iluwatar added the status: under review label Oct 13, 2019

iluwatar requested changes Oct 13, 2019

View reviewed changes

Remove Javadoc plugin to ensure correct build

79c2eda

iluwatar requested changes Oct 13, 2019

View reviewed changes

PalAditya added 2 commits October 14, 2019 02:08

Merge https://github.com/iluwatar/java-design-patterns

185c358

Implementing code review feedback

5d996f5

Merge master

cb6ac17

iluwatar requested changes Oct 14, 2019

View reviewed changes

Sync README with actual code

6686551

Merge master

52e7086

iluwatar approved these changes Oct 14, 2019

View reviewed changes

iluwatar added this to the 1.22.0 milestone Oct 14, 2019

iluwatar merged commit 7e698a9 into iluwatar:master Oct 14, 2019

		DelayedService delayedService = new DelayedService(20);
		String response = delayedService.response(serverStartTime);

Uh oh!

Conversation

PalAditya commented Oct 12, 2019

Uh oh!

iluwatar left a comment

Choose a reason for hiding this comment

Uh oh!

PalAditya commented Oct 13, 2019

Uh oh!

PalAditya commented Oct 13, 2019

Uh oh!

iluwatar commented Oct 13, 2019

Uh oh!

PalAditya commented Oct 13, 2019

Uh oh!

iluwatar commented Oct 13, 2019

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

iluwatar Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

PalAditya commented Oct 13, 2019

Uh oh!

iluwatar commented Oct 14, 2019

Uh oh!

PalAditya commented Oct 14, 2019

Uh oh!

PalAditya commented Oct 14, 2019

Uh oh!

iluwatar commented Oct 14, 2019

Uh oh!

PalAditya commented Oct 14, 2019

Uh oh!

iluwatar left a comment

Choose a reason for hiding this comment

Uh oh!

PalAditya commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluwatar commented Oct 14, 2019

Uh oh!

PalAditya commented Oct 14, 2019

Uh oh!

iluwatar commented Oct 14, 2019

Uh oh!

iluwatar commented Oct 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PalAditya commented Oct 14, 2019 •

edited

Loading