You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2020/09/27 18:01:44 UTC
[GitHub] [camel] djoleB opened a new pull request #4307: CAMEL-15585 - Camel Core health small refactor.
djoleB opened a new pull request #4307:
URL: https://github.com/apache/camel/pull/4307
[TASK](https://issues.apache.org/jira/browse/CAMEL-15585)
I found some things that i think should look a little better, no new functionality, just small refactor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] djoleB commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
djoleB commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495725485
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/RouteHealthCheck.java
##########
@@ -54,7 +54,8 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
if (route.getRouteController() != null || route.isAutoStartup()) {
if (status.isStarted()) {
builder.up();
- } else if (status.isStopped()) {
+ }
+ if (status.isStopped()) {
Review comment:
Okay, will revert that to previous state. Thanks @lburgazzoli :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] lburgazzoli commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495735046
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/ContextHealthCheck.java
##########
@@ -62,7 +62,9 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
if (camelContext.getStatus().isStarted()) {
builder.up();
- } else if (camelContext.getStatus().isStopped()) {
+ }
+
+ if (camelContext.getStatus().isStopped()) {
Review comment:
I think the else should be kept here too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] onderson commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
onderson commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495664499
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/DefaultHealthCheckRegistry.java
##########
@@ -166,66 +166,64 @@ private HealthCheckRepository resolveHealthCheckRepositoryById(String id) {
@Override
public boolean register(Object obj) {
- boolean accept = obj instanceof HealthCheck || obj instanceof HealthCheckRepository;
- if (!accept) {
- throw new IllegalArgumentException();
- }
+ checkIfAccepted(obj);
+
+ boolean result;
Review comment:
```suggestion
boolean result;
```
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/DefaultHealthCheckRegistry.java
##########
@@ -166,66 +166,64 @@ private HealthCheckRepository resolveHealthCheckRepositoryById(String id) {
@Override
public boolean register(Object obj) {
- boolean accept = obj instanceof HealthCheck || obj instanceof HealthCheckRepository;
- if (!accept) {
- throw new IllegalArgumentException();
- }
+ checkIfAccepted(obj);
Review comment:
```suggestion
boolean result;
checkIfAccepted(obj);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] lburgazzoli commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495728938
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/RouteHealthCheck.java
##########
@@ -54,7 +54,8 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
if (route.getRouteController() != null || route.isAutoStartup()) {
if (status.isStarted()) {
builder.up();
- } else if (status.isStopped()) {
+ }
+ if (status.isStopped()) {
Review comment:
I don't think it effectively changes the result in this case but an if else makes it clear that it is either started or stopped whereas a list of is could be confusing like can the status be `started` and `stopped` at the same time ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] lburgazzoli commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495723178
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/HealthCheckRegistryRepository.java
##########
@@ -109,14 +109,10 @@ private HealthCheck toHealthCheck(HealthCheck hc) {
}
private HealthCheckConfiguration matchConfiguration(String id) {
- if (configurations != null) {
- for (String key : configurations.keySet()) {
- if (PatternHelper.matchPattern(id, key)) {
- return configurations.get(key);
- }
- }
- }
- return fallbackConfiguration;
- }
+ return configurations.values().stream()
+ .filter(s -> PatternHelper.matchPattern(id, s.getParent()))
+ .findAny()
+ .orElseGet(() -> fallbackConfiguration);
Review comment:
I think `orElseGet(Supplier)` can be replaced by `orElse(fallbackConfiguration)` since the `fallbackConfiguration` does not need to be computed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] lburgazzoli commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495724795
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/RouteHealthCheck.java
##########
@@ -54,7 +54,8 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
if (route.getRouteController() != null || route.isAutoStartup()) {
if (status.isStarted()) {
builder.up();
- } else if (status.isStopped()) {
+ }
+ if (status.isStopped()) {
Review comment:
I's been long time since I've worked on this but I think the `else` should be preserved as if `status.isStarted()` returns true, there's no point to check also `status.isStopped()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] lburgazzoli commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495725667
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/RouteHealthCheck.java
##########
@@ -63,16 +64,16 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
// if a route is configured to not to automatically start, then the
// route is always up as it is externally managed.
builder.up();
- } else if (route.getRouteController() == null) {
+ }
+ if (route.getRouteController() == null) {
Review comment:
same as above
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #4307:
URL: https://github.com/apache/camel/pull/4307
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] djoleB commented on a change in pull request #4307: CAMEL-15585 - Camel Core health small refactor.
Posted by GitBox <gi...@apache.org>.
djoleB commented on a change in pull request #4307:
URL: https://github.com/apache/camel/pull/4307#discussion_r495739248
##########
File path: core/camel-health/src/main/java/org/apache/camel/impl/health/RouteHealthCheck.java
##########
@@ -54,7 +54,8 @@ protected void doCall(HealthCheckResultBuilder builder, Map<String, Object> opti
if (route.getRouteController() != null || route.isAutoStartup()) {
if (status.isStarted()) {
builder.up();
- } else if (status.isStopped()) {
+ }
+ if (status.isStopped()) {
Review comment:
Yes, i agree. Thank for clarifying. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org