You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "hungphan227 (via GitHub)" <gi...@apache.org> on 2024/09/05 03:48:45 UTC
[PR] JAMES-4068 Have several health checks in one rest calls [james-project]
hungphan227 opened a new pull request, #2399:
URL: https://github.com/apache/james-project/pull/2399
(no comment)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746578725
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -90,18 +88,26 @@ public void define(Service service) {
}
public Object validateHealthChecks(Request request, Response response) {
- Optional<String> maybeComponentNamesString = Optional.ofNullable(request.queryParams(QUERY_PARAM_COMPONENT_NAMES));
- Collection<HealthCheck> healthChecks = maybeComponentNamesString.map(this::getHealthChecks).orElse(healthCheckMap.values());
+ Set<ComponentName> selectedComponentNames =
+ Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
Review Comment:
The over way around is less hacky
```
Optionnal.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES))
.map(Stream::of)
.orElse(Stream.empty())
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745328094
##########
server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/HealthCheckRoutesTest.java:
##########
@@ -419,11 +578,25 @@ void getHealthchecksShouldReturnHealthCheckWhenHealthCheckPresent() {
void getHealthchecksShouldReturnHealthChecksWhenHealthChecksPresent() {
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_2)));
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_3)));
+ updateHealthCheckRoutes();
+
when()
.get(HealthCheckRoutes.CHECKS)
.then()
.body("", hasSize(2))
.statusCode(HttpStatus.OK_200);
}
+ private void updateHealthCheckRoutes() {
Review Comment:
**Evil**
Please do without it
If you need help here are some idea:
- nested tests
- extract a subcomponant and unit test it
- do not change the set data structure to a map.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745328094
##########
server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/HealthCheckRoutesTest.java:
##########
@@ -419,11 +578,25 @@ void getHealthchecksShouldReturnHealthCheckWhenHealthCheckPresent() {
void getHealthchecksShouldReturnHealthChecksWhenHealthChecksPresent() {
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_2)));
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_3)));
+ updateHealthCheckRoutes();
+
when()
.get(HealthCheckRoutes.CHECKS)
.then()
.body("", hasSize(2))
.statusCode(HttpStatus.OK_200);
}
+ private void updateHealthCheckRoutes() {
Review Comment:
**Evil**
Please do without it
If you need help here are some idea:
- nested tests
- extract a subcomponant and unit test it
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746659923
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -89,7 +90,7 @@ public void define(Service service) {
public Object validateHealthChecks(Request request, Response response) {
Set<ComponentName> selectedComponentNames =
- Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
+ Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).map(Stream::of).orElse(Stream.empty())
Review Comment:
Maybe we can be lucky enough to have good looking indent / method extraction / ???
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745109297
##########
docs/modules/servers/partials/operate/webadmin.adoc:
##########
@@ -94,6 +94,53 @@ Response codes:
services can still be used.
* 503: At least one check have answered with a Unhealthy status
+=== Check specific components
+
+Performs health checks for the given components. Components are
+referenced by their URL encoded names.
+
+....
+curl -XGET http://ip:port/healthcheck?checks={backend-name-1}+{backend-name}%20backend
Review Comment:
```suggestion
curl -XGET http://ip:port/healthcheck?checks=HealthCheck1+HealthCheck%20two
```
##########
docs/modules/servers/partials/operate/webadmin.adoc:
##########
@@ -94,6 +94,53 @@ Response codes:
services can still be used.
* 503: At least one check have answered with a Unhealthy status
+=== Check specific components
+
+Performs health checks for the given components. Components are
+referenced by their URL encoded names.
+
+....
+curl -XGET http://ip:port/healthcheck?checks={backend-name-1}+{backend-name}%20backend
Review Comment:
and just adapt below in the response example :)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746577540
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -90,18 +88,26 @@ public void define(Service service) {
}
public Object validateHealthChecks(Request request, Response response) {
- Optional<String> maybeComponentNamesString = Optional.ofNullable(request.queryParams(QUERY_PARAM_COMPONENT_NAMES));
- Collection<HealthCheck> healthChecks = maybeComponentNamesString.map(this::getHealthChecks).orElse(healthCheckMap.values());
+ Set<ComponentName> selectedComponentNames =
+ Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
+ .map(ComponentName::new)
+ .collect(ImmutableSet.toImmutableSet());
+ Collection<HealthCheck> selectedHealthChecks;
+ if (selectedComponentNames.isEmpty()) {
+ selectedHealthChecks = healthChecks;
Review Comment:
Variable reallocation spotted
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745128987
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -101,11 +113,25 @@ public Object performHealthCheckForComponent(Request request, Response response)
}
public Object getHealthChecks(Request request, Response response) {
- return healthChecks.stream()
- .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
- .collect(ImmutableList.toImmutableList());
+ return healthCheckMap.values().stream()
+ .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
+ .collect(ImmutableList.toImmutableList());
+ }
+
+ private Collection<HealthCheck> getHealthChecks(String componentNamesString) {
+ List<ComponentName> componentNames = Arrays.stream(componentNamesString.split("\\+")).map(ComponentName::new).toList();
Review Comment:
what happend if `component name` has "+" character?
IMO we can use a standard of http request
when check is an array
the syntax should be
`check=RabbitMQJmapEventBusDeadLetterQueueHealthCheck&check=OpenSearch%20Backend`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "hungphan227 (via GitHub)" <gi...@apache.org>.
hungphan227 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746509848
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
fixed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1744805902
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
How about keep using `Set<HealthCheck> healthChecks;`. We still can filter valid component names because the `HealthCheck` has `ComponentName` in itself. And remove the ugly and cheating reflection below.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745141297
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -101,11 +113,25 @@ public Object performHealthCheckForComponent(Request request, Response response)
}
public Object getHealthChecks(Request request, Response response) {
- return healthChecks.stream()
- .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
- .collect(ImmutableList.toImmutableList());
+ return healthCheckMap.values().stream()
+ .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
+ .collect(ImmutableList.toImmutableList());
+ }
+
+ private Collection<HealthCheck> getHealthChecks(String componentNamesString) {
+ List<ComponentName> componentNames = Arrays.stream(componentNamesString.split("\\+")).map(ComponentName::new).toList();
Review Comment:
> what happend if component name has "+" character?
I think the component can and likely should be URL encoded.
`OpenSearch+Backend` => `OpenSearch%2BBackend` ~ `checks=RabbitMQJmapEventBusDeadLetterQueueHealthCheck+OpenSearch%2BBackend` may be fine.
But it seems the `+` character has another special meaning in the URL:
cf: https://www.freecodecamp.org/news/url-encoded-characters-reference/#:~:text=There%20are%20only%20certain%20characters,that%20can%20have%20special%20meanings.
=> Likely we should avoid using the `+` as delimiter here as it does not mean a space.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "hungphan227 (via GitHub)" <gi...@apache.org>.
hungphan227 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745136107
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
discussed with @quantranhong1999 --> ignore
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1744902969
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
> iterating through the set is needed and the cost is perf.
Not that much IMO:
- the set size is small
- healthcheck does not need to be super performant
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "hungphan227 (via GitHub)" <gi...@apache.org>.
hungphan227 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1744900072
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
"We still can filter valid component names because the HealthCheck has ComponentName in itself." ----> with Set<HealthCheck> healthChecks, iterating through the set is needed and the cost is perf.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #2399:
URL: https://github.com/apache/james-project/pull/2399
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746909076
##########
docs/modules/servers/partials/operate/webadmin.adoc:
##########
@@ -100,7 +100,7 @@ Performs health checks for the given components. Components are
referenced by their URL encoded names.
....
-curl -XGET http://ip:port/healthcheck?checks={backend-name-1}+{backend-name}%20backend
+curl -XGET http://ip:port/healthcheck?check=HealthCheck1$check=HealthCheck%20two
Review Comment:
```suggestion
curl -XGET http://ip:port/healthcheck?check=HealthCheck1&check=HealthCheck%20two
```
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -90,18 +88,26 @@ public void define(Service service) {
}
public Object validateHealthChecks(Request request, Response response) {
- Optional<String> maybeComponentNamesString = Optional.ofNullable(request.queryParams(QUERY_PARAM_COMPONENT_NAMES));
- Collection<HealthCheck> healthChecks = maybeComponentNamesString.map(this::getHealthChecks).orElse(healthCheckMap.values());
+ Set<ComponentName> selectedComponentNames =
+ Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
+ .map(ComponentName::new)
+ .collect(ImmutableSet.toImmutableSet());
+ Collection<HealthCheck> selectedHealthChecks;
+ if (selectedComponentNames.isEmpty()) {
+ selectedHealthChecks = healthChecks;
Review Comment:
Not fixed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745110409
##########
server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/HealthCheckRoutesTest.java:
##########
@@ -298,10 +313,148 @@ void validateHealthChecksShouldReturnStatusUnHealthyWhenOneIsUnHealthyAndOtherIs
.when(Option.IGNORING_ARRAY_ORDER)
.isEqualTo(healthCheckBody);
}
+
+ @Test
+ void validateHealthChecksShouldReturnOkWhenSelectedHealthCheckIsHealthy() {
+ healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_1)));
+ healthChecks.add(healthCheck(Result.unhealthy(COMPONENT_NAME_2, "cause")));
+ updateHealthCheckRoutes();
+
+ String healthCheckBody =
+ "{\"status\": \"healthy\"," +
+ " \"checks\": [" +
+ " {" +
+ " \"componentName\": \"component-1\"," +
+ " \"escapedComponentName\": \"component-1\"," +
+ " \"status\": \"healthy\"," +
+ " \"cause\": null" +
+ "}]}";
+
+ String retrieveBody =
+ given()
+ .queryParam("checks", "component-1")
+ .when()
+ .get()
+ .then()
+ .statusCode(HttpStatus.OK_200)
+ .extract()
+ .body().asString();
+
+ assertThatJson(retrieveBody)
+ .when(Option.IGNORING_ARRAY_ORDER)
+ .isEqualTo(healthCheckBody);
+ }
+
+ @Test
+ void validateHealthChecksShouldReturnOkWhenSelectedHealthChecksAreHealthy() {
+ healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_1)));
+ healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_2)));
+ healthChecks.add(healthCheck(Result.unhealthy(COMPONENT_NAME_3, "cause")));
+ updateHealthCheckRoutes();
+
+ String healthCheckBody =
+ "{\"status\": \"healthy\"," +
+ " \"checks\": [" +
+ " {" +
+ " \"componentName\": \"component-1\"," +
+ " \"escapedComponentName\": \"component-1\"," +
+ " \"status\": \"healthy\"," +
+ " \"cause\": null" +
+ " }," +
+ " {" +
+ " \"componentName\": \"component-2\"," +
+ " \"escapedComponentName\": \"component-2\"," +
+ " \"status\": \"healthy\"," +
+ " \"cause\": null" +
+ "}]}";
+
+ String retrieveBody =
+ given()
+ .queryParam("checks", "component-1+component-2")
+ .when()
+ .get()
+ .then()
+ .statusCode(HttpStatus.OK_200)
+ .extract()
+ .body().asString();
+
+ assertThatJson(retrieveBody)
+ .when(Option.IGNORING_ARRAY_ORDER)
+ .isEqualTo(healthCheckBody);
+ }
+
+ @Test
+ void validateHealthChecksShouldReturnStatusUnHealthyWhenOneOfSelectedHealthChecksIsUnhealthy() {
+ healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_1)));
+ healthChecks.add(healthCheck(Result.unhealthy(COMPONENT_NAME_2, "cause")));
+ updateHealthCheckRoutes();
+
+ String healthCheckBody =
+ "{\"status\": \"unhealthy\"," +
+ " \"checks\": [" +
+ " {" +
+ " \"componentName\": \"component-1\"," +
+ " \"escapedComponentName\": \"component-1\"," +
+ " \"status\": \"healthy\"," +
+ " \"cause\": null" +
+ " }," +
+ " {" +
+ " \"componentName\": \"component-2\"," +
+ " \"escapedComponentName\": \"component-2\"," +
+ " \"status\": \"unhealthy\"," +
+ " \"cause\": \"cause\"" +
+ "}]}";
+
+ String retrieveBody =
+ given()
+ .queryParam("checks", "component-1+component-2")
+ .when()
+ .get()
+ .then()
+ .statusCode(HttpStatus.SERVICE_UNAVAILABLE_503)
+ .extract()
+ .body().asString();
+
+ assertThatJson(retrieveBody)
+ .when(Option.IGNORING_ARRAY_ORDER)
+ .isEqualTo(healthCheckBody);
+ }
+
+ @Test
+ void validateHealthChecksShouldReturnNotFoundWhenOneOfSelectedHealthChecksIsUnknown() {
+ healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_1)));
+ updateHealthCheckRoutes();
+
+ given()
+ .queryParam("checks", "component-1+component-unknown")
+ .when()
+ .get()
+ .then()
+ .statusCode(HttpStatus.NOT_FOUND_404)
+ .body("details", is(nullValue()))
+ .body("type", equalTo("notFound"))
+ .body("message", equalTo("Components with name component-unknown cannot be found"))
+ .body("statusCode", is(HttpStatus.NOT_FOUND_404));
+ }
+
+ @Test
+ void validateHealthChecksShouldReturnNotFoundWhenAllSelectedHealthChecksIsUnknown() {
+ given()
+ .queryParam("checks", "component-unknown-1+component-unknown-2")
+ .when()
Review Comment:
indent
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1745328094
##########
server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/HealthCheckRoutesTest.java:
##########
@@ -419,11 +578,25 @@ void getHealthchecksShouldReturnHealthCheckWhenHealthCheckPresent() {
void getHealthchecksShouldReturnHealthChecksWhenHealthChecksPresent() {
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_2)));
healthChecks.add(healthCheck(Result.healthy(COMPONENT_NAME_3)));
+ updateHealthCheckRoutes();
+
when()
.get(HealthCheckRoutes.CHECKS)
.then()
.body("", hasSize(2))
.statusCode(HttpStatus.OK_200);
}
+ private void updateHealthCheckRoutes() {
Review Comment:
**Evil**
Please do without it
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -101,11 +113,25 @@ public Object performHealthCheckForComponent(Request request, Response response)
}
public Object getHealthChecks(Request request, Response response) {
- return healthChecks.stream()
- .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
- .collect(ImmutableList.toImmutableList());
+ return healthCheckMap.values().stream()
+ .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
+ .collect(ImmutableList.toImmutableList());
+ }
+
+ private Collection<HealthCheck> getHealthChecks(String componentNamesString) {
+ List<ComponentName> componentNames = Arrays.stream(componentNamesString.split("\\+")).map(ComponentName::new).toList();
Review Comment:
Splitter.on(...).splitToList(...)
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -56,15 +63,17 @@ public class HealthCheckRoutes implements PublicRoutes {
public static final String HEALTHCHECK = "/healthcheck";
public static final String CHECKS = "/checks";
-
+
private static final String PARAM_COMPONENT_NAME = "componentName";
+ private static final String QUERY_PARAM_COMPONENT_NAMES = "checks";
private final JsonTransformer jsonTransformer;
- private final Set<HealthCheck> healthChecks;
+ private final Map<ComponentName, HealthCheck> healthCheckMap;
Review Comment:
> discussed with @quantranhong1999 --> ignore
Why?
I was having same comment...
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "hungphan227 (via GitHub)" <gi...@apache.org>.
hungphan227 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746510941
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -101,11 +113,25 @@ public Object performHealthCheckForComponent(Request request, Response response)
}
public Object getHealthChecks(Request request, Response response) {
- return healthChecks.stream()
- .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
- .collect(ImmutableList.toImmutableList());
+ return healthCheckMap.values().stream()
+ .map(healthCheck -> new HealthCheckDto(healthCheck.componentName()))
+ .collect(ImmutableList.toImmutableList());
+ }
+
+ private Collection<HealthCheck> getHealthChecks(String componentNamesString) {
+ List<ComponentName> componentNames = Arrays.stream(componentNamesString.split("\\+")).map(ComponentName::new).toList();
Review Comment:
updated: check=RabbitMQJmapEventBusDeadLetterQueueHealthCheck&check=OpenSearch%20Backend
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746578725
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -90,18 +88,26 @@ public void define(Service service) {
}
public Object validateHealthChecks(Request request, Response response) {
- Optional<String> maybeComponentNamesString = Optional.ofNullable(request.queryParams(QUERY_PARAM_COMPONENT_NAMES));
- Collection<HealthCheck> healthChecks = maybeComponentNamesString.map(this::getHealthChecks).orElse(healthCheckMap.values());
+ Set<ComponentName> selectedComponentNames =
+ Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
Review Comment:
The other way around is less hacky
```
Optionnal.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES))
.map(Stream::of)
.orElse(Stream.empty())
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4068 Have several health checks in one rest calls [james-project]
Posted by "hungphan227 (via GitHub)" <gi...@apache.org>.
hungphan227 commented on code in PR #2399:
URL: https://github.com/apache/james-project/pull/2399#discussion_r1746691591
##########
server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/HealthCheckRoutes.java:
##########
@@ -89,7 +90,7 @@ public void define(Service service) {
public Object validateHealthChecks(Request request, Response response) {
Set<ComponentName> selectedComponentNames =
- Arrays.stream(Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).orElse(new String[0]))
+ Optional.ofNullable(request.queryParamsValues(QUERY_PARAM_COMPONENT_NAMES)).map(Stream::of).orElse(Stream.empty())
Review Comment:
updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org