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 2022/03/16 02:28:18 UTC
[GitHub] [pulsar] wangjialing218 opened a new pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
wangjialing218 opened a new pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702
### Motivation
In order to list brokers in cluster, we need pass cluster name to admin api, although we only have one local cluster.
In case of get the broker list for local cluster, we could ignore the cluster name just like other admin api.
### Modifications
Add admin api to get broker list for local cluster without cluster name.
### Verifying this change
This change added tests and can be verified as follows:
AdminApiTest
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- The public API: (yes)
- The admin cli options: (yes)
### Documentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?
- [x] `doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828548262
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Most of the codes are duplicated to
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster)
```
How about abstract a common method used by both
```
getActiveBrokers(@Suspended final AsyncResponse asyncResponse, PathParam("cluster") String cluster)
```
and
```
getActiveBrokers(@Suspended final AsyncResponse asyncResponse)
` ``
?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828668102
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Good suggestion, I removed duplicated code
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829643219
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +105,21 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ getActiveBrokers(asyncResponse, null);
+ }
+
Review comment:
Why add this endpoint ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1077200129
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1082581436
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829649359
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -89,7 +89,8 @@
public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster) {
validateSuperUserAccessAsync()
- .thenCompose(__ -> validateClusterOwnershipAsync(cluster))
+ .thenCompose(__ -> cluster == null ? CompletableFuture.completedFuture(null)
+ : validateClusterOwnershipAsync(cluster))
Review comment:
Yes, `validateClusterOwnershipAsync` could also pass when cluster=null
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -42,14 +42,24 @@ public BrokersImpl(WebTarget web, Authentication auth, long readTimeoutMs) {
adminBrokers = web.path("admin/v2/brokers");
}
+ @Override
+ public List<String> getActiveBrokers() throws PulsarAdminException {
+ return sync(() -> getActiveBrokersAsync(null));
+ }
+
+ @Override
+ public CompletableFuture<List<String>> getActiveBrokersAsync() {
+ return getActiveBrokersAsync(null);
+ }
+
@Override
public List<String> getActiveBrokers(String cluster) throws PulsarAdminException {
return sync(() -> getActiveBrokersAsync(cluster));
}
@Override
public CompletableFuture<List<String>> getActiveBrokersAsync(String cluster) {
- WebTarget path = adminBrokers.path(cluster);
+ WebTarget path = cluster == null ? adminBrokers : adminBrokers.path(cluster);
Review comment:
`WebTarget.path` does not support null param, othewise it`ll get "path is 'null'."
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1074698379
@Technoboy- Please help review again.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829649359
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -89,7 +89,8 @@
public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster) {
validateSuperUserAccessAsync()
- .thenCompose(__ -> validateClusterOwnershipAsync(cluster))
+ .thenCompose(__ -> cluster == null ? CompletableFuture.completedFuture(null)
+ : validateClusterOwnershipAsync(cluster))
Review comment:
Yes, `validateClusterOwnershipAsync` could also pass when cluster=null
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -42,14 +42,24 @@ public BrokersImpl(WebTarget web, Authentication auth, long readTimeoutMs) {
adminBrokers = web.path("admin/v2/brokers");
}
+ @Override
+ public List<String> getActiveBrokers() throws PulsarAdminException {
+ return sync(() -> getActiveBrokersAsync(null));
+ }
+
+ @Override
+ public CompletableFuture<List<String>> getActiveBrokersAsync() {
+ return getActiveBrokersAsync(null);
+ }
+
@Override
public List<String> getActiveBrokers(String cluster) throws PulsarAdminException {
return sync(() -> getActiveBrokersAsync(cluster));
}
@Override
public CompletableFuture<List<String>> getActiveBrokersAsync(String cluster) {
- WebTarget path = adminBrokers.path(cluster);
+ WebTarget path = cluster == null ? adminBrokers : adminBrokers.path(cluster);
Review comment:
`WebTarget.path` does not support null param, othewise it`ll get "path is 'null'."
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828548262
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Most of the codes are duplicated to
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster)
```
How about abstract a common method used by both
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse, PathParam("cluster") String cluster)
```
and
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse)
` ``
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Most of the codes are duplicated to
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster)
```
How about abstract a common method used by both
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse, PathParam("cluster") String cluster)
```
and
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse)
` ``
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828548262
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Most of the codes are duplicated to
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster)
```
How about abstract a common method used by both the `getActiveBrokers(@Suspended final AsyncResponse asyncResponse, PathParam("cluster") String cluster)` and `getActiveBrokers(@Suspended final AsyncResponse asyncResponse)` ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1082678927
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1081348841
@wangjialing218
1. you do not need to update `reference-pulsar-admin.md` because it is deprecated
2. your doc changes will be shown on admin-api docs since you've added explanations to code files
3. if you want to draw more attention to your changes, you can update docs here https://pulsar.apache.org/docs/en/next/admin-api-brokers/#brokers-resources
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1082530244
@Anonymitaet
rollback change to `reference-pulsar-admin.md`, and update `admin-api-brokers.md`, please have a look
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r827800429
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -32,6 +32,41 @@
* Admin interface for brokers management.
*/
public interface Brokers {
+ /**
+ * Get the list of active brokers in the local cluster.
+ * <p/>
+ * Get the list of active brokers (web service addresses) in the local cluster.
+ * <p/>
+ * Response Example:
+ *
+ * <pre>
+ * <code>["prod1-broker1.messaging.use.example.com:8080", "prod1-broker2.messaging.use.example.com:8080"
+ * * * "prod1-broker3.messaging.use.example.com:8080"]</code>
+ * </pre>
+ *
+ * @return a list of (host:port)
+ * @throws NotAuthorizedException
+ * You don't have admin permission to get the list of active brokers in the cluster
+ * @throws PulsarAdminException
+ * Unexpected error
+ */
+ List<String> getActiveBrokers() throws PulsarAdminException;
Review comment:
I think we can continue to use `getActiveBrokers(String cluster)`.
When the `cluster` is `null`, you can request `/`, otherwise, you can request `/{cluster}`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828548262
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +104,30 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> pulsar().getLoadManager().get().getAvailableBrokersAsync())
+ .thenAccept(activeBrokers -> {
+ LOG.info("[{}] Successfully to get active brokers", clientAppId());
+ asyncResponse.resume(activeBrokers);
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Fail to get active brokers", clientAppId(), ex);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
+ return null;
+ });
Review comment:
Most of the codes are duplicated to
```java
getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster)
```
How about abstracting a common method used by both these two methods?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829643604
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -32,6 +32,41 @@
* Admin interface for brokers management.
*/
public interface Brokers {
+ /**
+ * Get the list of active brokers in the local cluster.
+ * <p/>
+ * Get the list of active brokers (web service addresses) in the local cluster.
+ * <p/>
+ * Response Example:
+ *
+ * <pre>
+ * <code>["prod1-broker1.messaging.use.example.com:8080", "prod1-broker2.messaging.use.example.com:8080"
+ * * * "prod1-broker3.messaging.use.example.com:8080"]</code>
+ * </pre>
+ *
+ * @return a list of (host:port)
+ * @throws NotAuthorizedException
+ * You don't have admin permission to get the list of active brokers in the cluster
+ * @throws PulsarAdminException
+ * Unexpected error
+ */
+ List<String> getActiveBrokers() throws PulsarAdminException;
Review comment:
Yes right. But seems no need to modify other part.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] momo-jun commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
momo-jun commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1080649167
Hi @wangjialing218
It seems that the `reference-pulsar-admin.md` file you modified has already been deprecated. You can modify the `CmdBroker.java` file under the pulsar/blob/master/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/ directory instead to make the change.
cc @Anonymitaet. Feel free to correct me if anything is wrong.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r828670981
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -32,6 +32,41 @@
* Admin interface for brokers management.
*/
public interface Brokers {
+ /**
+ * Get the list of active brokers in the local cluster.
+ * <p/>
+ * Get the list of active brokers (web service addresses) in the local cluster.
+ * <p/>
+ * Response Example:
+ *
+ * <pre>
+ * <code>["prod1-broker1.messaging.use.example.com:8080", "prod1-broker2.messaging.use.example.com:8080"
+ * * * "prod1-broker3.messaging.use.example.com:8080"]</code>
+ * </pre>
+ *
+ * @return a list of (host:port)
+ * @throws NotAuthorizedException
+ * You don't have admin permission to get the list of active brokers in the cluster
+ * @throws PulsarAdminException
+ * Unexpected error
+ */
+ List<String> getActiveBrokers() throws PulsarAdminException;
Review comment:
User coud also use `getActiveBrokers(null)` to get broker list for local cluster, but it's a littile strange, so I add `getActiveBrokers()`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829643380
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -42,14 +42,24 @@ public BrokersImpl(WebTarget web, Authentication auth, long readTimeoutMs) {
adminBrokers = web.path("admin/v2/brokers");
}
+ @Override
+ public List<String> getActiveBrokers() throws PulsarAdminException {
+ return sync(() -> getActiveBrokersAsync(null));
+ }
+
+ @Override
+ public CompletableFuture<List<String>> getActiveBrokersAsync() {
+ return getActiveBrokersAsync(null);
+ }
+
@Override
public List<String> getActiveBrokers(String cluster) throws PulsarAdminException {
return sync(() -> getActiveBrokersAsync(cluster));
}
@Override
public CompletableFuture<List<String>> getActiveBrokersAsync(String cluster) {
- WebTarget path = adminBrokers.path(cluster);
+ WebTarget path = cluster == null ? adminBrokers : adminBrokers.path(cluster);
Review comment:
Seems no need to modify
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829642912
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -89,7 +89,8 @@
public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster) {
validateSuperUserAccessAsync()
- .thenCompose(__ -> validateClusterOwnershipAsync(cluster))
+ .thenCompose(__ -> cluster == null ? CompletableFuture.completedFuture(null)
+ : validateClusterOwnershipAsync(cluster))
Review comment:
No need to modify this line.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -104,6 +105,21 @@ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
});
}
+ @GET
+ @Path("/")
+ @ApiOperation(
+ value = "Get the list of active brokers (web service addresses) in the local cluster."
+ + "If authorization is not enabled",
+ response = String.class,
+ responseContainer = "Set")
+ @ApiResponses(
+ value = {
+ @ApiResponse(code = 401, message = "Authentication required"),
+ @ApiResponse(code = 403, message = "This operation requires super-user access") })
+ public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse) throws Exception {
+ getActiveBrokers(asyncResponse, null);
+ }
+
Review comment:
Why add this endpoint ?
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java
##########
@@ -42,14 +42,24 @@ public BrokersImpl(WebTarget web, Authentication auth, long readTimeoutMs) {
adminBrokers = web.path("admin/v2/brokers");
}
+ @Override
+ public List<String> getActiveBrokers() throws PulsarAdminException {
+ return sync(() -> getActiveBrokersAsync(null));
+ }
+
+ @Override
+ public CompletableFuture<List<String>> getActiveBrokersAsync() {
+ return getActiveBrokersAsync(null);
+ }
+
@Override
public List<String> getActiveBrokers(String cluster) throws PulsarAdminException {
return sync(() -> getActiveBrokersAsync(cluster));
}
@Override
public CompletableFuture<List<String>> getActiveBrokersAsync(String cluster) {
- WebTarget path = adminBrokers.path(cluster);
+ WebTarget path = cluster == null ? adminBrokers : adminBrokers.path(cluster);
Review comment:
Seems no need to modify
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Brokers.java
##########
@@ -32,6 +32,41 @@
* Admin interface for brokers management.
*/
public interface Brokers {
+ /**
+ * Get the list of active brokers in the local cluster.
+ * <p/>
+ * Get the list of active brokers (web service addresses) in the local cluster.
+ * <p/>
+ * Response Example:
+ *
+ * <pre>
+ * <code>["prod1-broker1.messaging.use.example.com:8080", "prod1-broker2.messaging.use.example.com:8080"
+ * * * "prod1-broker3.messaging.use.example.com:8080"]</code>
+ * </pre>
+ *
+ * @return a list of (host:port)
+ * @throws NotAuthorizedException
+ * You don't have admin permission to get the list of active brokers in the cluster
+ * @throws PulsarAdminException
+ * Unexpected error
+ */
+ List<String> getActiveBrokers() throws PulsarAdminException;
Review comment:
Yes right. But seems no need to modify other part.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#discussion_r829642912
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -89,7 +89,8 @@
public void getActiveBrokers(@Suspended final AsyncResponse asyncResponse,
@PathParam("cluster") String cluster) {
validateSuperUserAccessAsync()
- .thenCompose(__ -> validateClusterOwnershipAsync(cluster))
+ .thenCompose(__ -> cluster == null ? CompletableFuture.completedFuture(null)
+ : validateClusterOwnershipAsync(cluster))
Review comment:
No need to modify this line.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1080106488
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 commented on pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702#issuecomment-1085291254
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wangjialing218 closed pull request #14702: [PulsarAdmin] add get active brokers api without cluster name
Posted by GitBox <gi...@apache.org>.
wangjialing218 closed pull request #14702:
URL: https://github.com/apache/pulsar/pull/14702
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org