You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:26:55 UTC
[GitHub] [pulsar] rdhabalia commented on a change in pull request #10962: [WIP] Add v2 health admin endpoint
rdhabalia commented on a change in pull request #10962:
URL: https://github.com/apache/pulsar/pull/10962#discussion_r653920532
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,19 +292,19 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
+ private void healthCheck(AsyncResponse asyncResponse,
+ TopicVersion topicVersion) throws PulsarServerException {
+
validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ String topic;
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ topic = ""; // TODO
Review comment:
please remove TODO
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,31 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/healthv2")
Review comment:
if you have different behavior of endpoint based on topic name then
v2 endpoint should go under: `org.apache.pulsar.broker.admin.v2.Brokers` and v1 should go under: `org.apache.pulsar.broker.admin.v1.Brokers`
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,19 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/health")
Review comment:
shouldn't this move to v1-brokers.?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -371,6 +373,19 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
asyncResponse.resume("ok");
}
});
+
+ }
+
+ @GET
+ @Path("/health")
Review comment:
or we can use just a query-param with topic format, if it doesn't exist then default behavior , if it does then it formats topic based on the format. that might be simple and clean change?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +387,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
Review comment:
I guess, we can avoid separate `healthCheck` method and can achieve with one line code change.
```
String namespace = (topicVersion == TopicVersion.V2) ? NamespaceService.getHeartbeatNamespaceV2(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration()) : NamespaceService.getHeartbeatNamespace(
pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
:
// and then same logic..
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,30 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic = null;
+ PulsarClient client = null;
+ try {
+ validateSuperUserAccess();
+
+ if (topicVersion == TopicVersion.V1) {
+ String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
+ pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
+ topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ } else {
+ LOG.info("Using healthCheck with V2 topic name");
Review comment:
I guess you can just add generic info log with topic name once you conclude topic eg:
`LOG.info("Doing healthCheck for {}", topic);`
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBrokers.java
##########
@@ -127,9 +128,19 @@ void run() throws Exception {
@Parameters(commandDescription = "Run a health check against the broker")
private class HealthcheckCmd extends CliCommand {
+ @Parameter(names = "--topic-version", description = "topic name version [V1,V2] V1 is default")
+ private String topicVersion;
Review comment:
make it TopicVersion enum instead string.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBrokers.java
##########
@@ -127,9 +128,19 @@ void run() throws Exception {
@Parameters(commandDescription = "Run a health check against the broker")
private class HealthcheckCmd extends CliCommand {
+ @Parameter(names = "--topic-version", description = "topic name version [V1,V2] V1 is default")
+ private String topicVersion;
+
@Override
void run() throws Exception {
- getAdmin().brokers().healthcheck();
+ if (Objects.isNull(topicVersion) || topicVersion.equalsIgnoreCase("V1")) {
+ getAdmin().brokers().healthcheck("V1");
Review comment:
if we take enum as an input argument then we don't have to validate. we can just call
`getAdmin().brokers().healthcheck(topicVersion);`
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
Review comment:
same here
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +368,31 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
+ @Deprecated
public CompletableFuture<Void> healthcheckAsync() {
+ return healthcheckAsync("V1");
+ }
+
+ @Override
+ public void healthcheck(String topicVersion) throws PulsarAdminException {
Review comment:
keep input type`TopicVersion` instead `String`
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -352,9 +353,10 @@ public void failed(Throwable throwable) {
}
@Override
+ @Deprecated
public void healthcheck() throws PulsarAdminException {
try {
- healthcheckAsync().get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
+ healthcheckAsync("V1").get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
Review comment:
query param is optional and if param is null then it should consider V1 by default. so, we should not change the behavior and let it be empty which can validate that the existing behavior is working.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
we don't need any extra conditional statements anymore because protected-healthcheck method already handles topic-version type. we should remove this method and make protected-healthcheck method main public-method now because it handles all scenarios.
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -366,8 +380,11 @@ public void healthcheck() throws PulsarAdminException {
}
@Override
- public CompletableFuture<Void> healthcheckAsync() {
+ public CompletableFuture<Void> healthcheckAsync(TopicVersion topicVersion) {
WebTarget path = adminBrokers.path("health");
+ if (Objects.nonNull(topicVersion)) {
Review comment:
any reason by just not checking `topicVersion != null`, and avoiding extra method call.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
here we are not doing any check based on topic name unlike topic/namespace. therefore, adding just a query param should be fine and cleaner approach.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -290,21 +295,34 @@ public void isReady(@Suspended AsyncResponse asyncResponse) {
}
}
- @GET
- @Path("/health")
- @ApiOperation(value = "Run a healthcheck against the broker")
- @ApiResponses(value = {
- @ApiResponse(code = 200, message = "Everything is OK"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Cluster doesn't exist"),
- @ApiResponse(code = 500, message = "Internal server error")})
- public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception {
- validateSuperUserAccess();
- String heartbeatNamespace = NamespaceService.getHeartbeatNamespace(
- pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
- String topic = String.format("persistent://%s/healthcheck", heartbeatNamespace);
+ protected void healthCheck(final AsyncResponse asyncResponse,
+ final TopicVersion topicVersion) throws PulsarServerException {
+
+ String topic;
+ PulsarClient client;
+ try {
+ validateSuperUserAccess();
+ String heartbeatNamespace;
+
+ heartbeatNamespace = (topicVersion == TopicVersion.V1)
Review comment:
sorry I read @cckellogg comment incorrectly. Yes, we should check against v2
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -373,6 +391,26 @@ public void healthcheck(@Suspended AsyncResponse asyncResponse) throws Exception
});
}
+ @GET
+ @Path("/health")
+ @ApiOperation(value = "Run a healthcheck against the broker")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Everything is OK"),
+ @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Cluster doesn't exist"),
+ @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiParam(value = "Topic Version")
+ public void healthcheck(@Suspended AsyncResponse asyncResponse,
+ @QueryParam("topicversion") TopicVersion topicVersion) throws Exception {
+ if (Objects.isNull(topicVersion) || topicVersion == TopicVersion.V1) {
+ healthCheck(asyncResponse, TopicVersion.V1);
+ } else if (topicVersion == TopicVersion.V2) {
Review comment:
it already handles null if you derive namespace
```
String namespace = (topicVersion == TopicVersion.V2) ? NamespaceService.getHeartbeatNamespaceV2( pulsar().getAdvertisedAddress(), pulsar().getConfiguration()) : NamespaceService.getHeartbeatNamespace( pulsar().getAdvertisedAddress(), pulsar().getConfiguration());
```
--
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