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/04/27 10:49:42 UTC
[GitHub] [pulsar] nodece opened a new pull request, #15348: [improve][broker] Make internalGetStats to async
nodece opened a new pull request, #15348:
URL: https://github.com/apache/pulsar/pull/15348
### Motivation
Avoid using the synchronization method in `internalGetStats()`.
### Modifications
- Use the asynchronous method instead of the synchronization method
### Documentation
Need to update docs?
- [x] `no-need-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] Technoboy- merged pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15348:
URL: https://github.com/apache/pulsar/pull/15348
--
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 pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#issuecomment-1112981880
/pulsarbot rerun-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] nodece commented on pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#issuecomment-1112331584
/pulsarbot rerun-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] Technoboy- commented on a diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r862277094
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java:
##########
@@ -1042,7 +1042,16 @@ public TopicStats getStats(
@ApiParam(value = "If return time of the earliest message in backlog")
@QueryParam("getEarliestTimeInBacklog") @DefaultValue("false") boolean getEarliestTimeInBacklog) {
validateTopicName(tenant, namespace, encodedTopic);
- return internalGetStats(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog);
+ internalGetStatsAsync(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog)
Review Comment:
Hi @nodece , I have checked that `validateTopicName` already throw RestException, no need to use try/catch.
--
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 diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r861712166
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java:
##########
@@ -1042,7 +1042,16 @@ public TopicStats getStats(
@ApiParam(value = "If return time of the earliest message in backlog")
@QueryParam("getEarliestTimeInBacklog") @DefaultValue("false") boolean getEarliestTimeInBacklog) {
validateTopicName(tenant, namespace, encodedTopic);
- return internalGetStats(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog);
+ internalGetStatsAsync(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog)
Review Comment:
We need to use try/catch block to wrap `validateTopicName`.
--
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 diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r862447732
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java:
##########
@@ -364,13 +363,24 @@ public void getSubscriptions(@Suspended final AsyncResponse asyncResponse, @Path
@ApiResponses(value = {
@ApiResponse(code = 307, message = "Current broker doesn't serve the namespace of this topic"),
@ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Topic does not exist") })
- public TopicStats getStats(@PathParam("property") String property, @PathParam("cluster") String cluster,
+ @ApiResponse(code = 404, message = "Topic does not exist")})
+ public void getStats(
+ @Suspended final AsyncResponse asyncResponse,
+ @PathParam("property") String property, @PathParam("cluster") String cluster,
@PathParam("namespace") String namespace, @PathParam("topic") @Encoded String encodedTopic,
@QueryParam("authoritative") @DefaultValue("false") boolean authoritative,
@QueryParam("getPreciseBacklog") @DefaultValue("false") boolean getPreciseBacklog) {
validateTopicName(property, cluster, namespace, encodedTopic);
- return internalGetStats(authoritative, getPreciseBacklog, false, false);
+ internalGetStatsAsync(authoritative, getPreciseBacklog, false, false)
+ .thenAccept(asyncResponse::resume)
+ .exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
Review Comment:
Keep style with old 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] nodece commented on a diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r860655584
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -1162,22 +1162,29 @@ private void internalGetSubscriptionsForNonPartitionedTopic(AsyncResponse asyncR
});
}
- protected TopicStats internalGetStats(boolean authoritative, boolean getPreciseBacklog,
- boolean subscriptionBacklogSize, boolean getEarliestTimeInBacklog) {
+ protected void internalGetStatsAsync(AsyncResponse asyncResponse, boolean authoritative, boolean getPreciseBacklog,
Review Comment:
@codelipenghui 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r862279315
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java:
##########
@@ -364,13 +363,24 @@ public void getSubscriptions(@Suspended final AsyncResponse asyncResponse, @Path
@ApiResponses(value = {
@ApiResponse(code = 307, message = "Current broker doesn't serve the namespace of this topic"),
@ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Topic does not exist") })
- public TopicStats getStats(@PathParam("property") String property, @PathParam("cluster") String cluster,
+ @ApiResponse(code = 404, message = "Topic does not exist")})
+ public void getStats(
+ @Suspended final AsyncResponse asyncResponse,
+ @PathParam("property") String property, @PathParam("cluster") String cluster,
@PathParam("namespace") String namespace, @PathParam("topic") @Encoded String encodedTopic,
@QueryParam("authoritative") @DefaultValue("false") boolean authoritative,
@QueryParam("getPreciseBacklog") @DefaultValue("false") boolean getPreciseBacklog) {
validateTopicName(property, cluster, namespace, encodedTopic);
- return internalGetStats(authoritative, getPreciseBacklog, false, false);
+ internalGetStatsAsync(authoritative, getPreciseBacklog, false, false)
+ .thenAccept(asyncResponse::resume)
+ .exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
Review Comment:
This comment seems not neccessary.
--
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 a diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r866778679
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java:
##########
@@ -89,27 +87,6 @@ public PartitionedTopicMetadata getPartitionedMetadata(@PathParam("property") St
return getPartitionedTopicMetadata(topicName, authoritative, checkAllowAutoCreation);
}
- @GET
- @Path("{property}/{cluster}/{namespace}/{topic}/stats")
- @ApiOperation(hidden = true, value = "Get the stats for the topic.")
- @ApiResponses(value = {
- @ApiResponse(code = 307, message = "Current broker doesn't serve the namespace of this topic"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Topic does not exist")})
- public NonPersistentTopicStats getStats(@PathParam("property") String property,
- @PathParam("cluster") String cluster,
- @PathParam("namespace") String namespace,
- @PathParam("topic") @Encoded String encodedTopic,
- @QueryParam("authoritative") @DefaultValue("false") boolean authoritative,
- @QueryParam("getPreciseBacklog") @DefaultValue("false")
- boolean getPreciseBacklog) {
- validateTopicName(property, cluster, namespace, encodedTopic);
- validateTopicOwnership(topicName, authoritative);
- validateTopicOperation(topicName, TopicOperation.GET_STATS);
- Topic topic = getTopicReference(topicName);
- return ((NonPersistentTopic) topic).getStats(getPreciseBacklog, false, false);
- }
-
Review Comment:
Do we have tests for a non-persistent topic? To make sure we will not introduce any break changes.
--
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 pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#issuecomment-1120763249
/pulsarbot rerun-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] codelipenghui commented on a diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r860391619
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -1162,22 +1162,29 @@ private void internalGetSubscriptionsForNonPartitionedTopic(AsyncResponse asyncR
});
}
- protected TopicStats internalGetStats(boolean authoritative, boolean getPreciseBacklog,
- boolean subscriptionBacklogSize, boolean getEarliestTimeInBacklog) {
+ protected void internalGetStatsAsync(AsyncResponse asyncResponse, boolean authoritative, boolean getPreciseBacklog,
Review Comment:
Same as this comment https://github.com/apache/pulsar/pull/15347#discussion_r860391215
--
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 diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r861921604
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java:
##########
@@ -1042,7 +1042,16 @@ public TopicStats getStats(
@ApiParam(value = "If return time of the earliest message in backlog")
@QueryParam("getEarliestTimeInBacklog") @DefaultValue("false") boolean getEarliestTimeInBacklog) {
validateTopicName(tenant, namespace, encodedTopic);
- return internalGetStats(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog);
+ internalGetStatsAsync(authoritative, getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog)
Review Comment:
Good point, I suggest we should add an global exception hook to handle this.
--
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 diff in pull request #15348: [improve][broker] Make internalGetStats to async
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15348:
URL: https://github.com/apache/pulsar/pull/15348#discussion_r867325123
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/NonPersistentTopics.java:
##########
@@ -89,27 +87,6 @@ public PartitionedTopicMetadata getPartitionedMetadata(@PathParam("property") St
return getPartitionedTopicMetadata(topicName, authoritative, checkAllowAutoCreation);
}
- @GET
- @Path("{property}/{cluster}/{namespace}/{topic}/stats")
- @ApiOperation(hidden = true, value = "Get the stats for the topic.")
- @ApiResponses(value = {
- @ApiResponse(code = 307, message = "Current broker doesn't serve the namespace of this topic"),
- @ApiResponse(code = 403, message = "Don't have admin permission"),
- @ApiResponse(code = 404, message = "Topic does not exist")})
- public NonPersistentTopicStats getStats(@PathParam("property") String property,
- @PathParam("cluster") String cluster,
- @PathParam("namespace") String namespace,
- @PathParam("topic") @Encoded String encodedTopic,
- @QueryParam("authoritative") @DefaultValue("false") boolean authoritative,
- @QueryParam("getPreciseBacklog") @DefaultValue("false")
- boolean getPreciseBacklog) {
- validateTopicName(property, cluster, namespace, encodedTopic);
- validateTopicOwnership(topicName, authoritative);
- validateTopicOperation(topicName, TopicOperation.GET_STATS);
- Topic topic = getTopicReference(topicName);
- return ((NonPersistentTopic) topic).getStats(getPreciseBacklog, false, false);
- }
-
Review Comment:
@codelipenghui I add a test for this, please help review, thanks.
--
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