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/02/18 02:02:09 UTC
[GitHub] [pulsar] Technoboy- opened a new issue #14365: [DISCUSS] Standardize Admin REST API
Technoboy- opened a new issue #14365:
URL: https://github.com/apache/pulsar/issues/14365
### Motivation
The Rest API was originally designed to be implemented asynchronously, but with the iteration of functions, some synchronous implementations were added, resulting in many asynchronous methods calling synchronous implementations. Also, many synchronous calls do not add timeouts. This greatly reduces concurrency, user operations and experience. At the same time, new issues are also introduced, such as Apache/Pulsar#13885.
In order to prevent more problems, and improve code readability and maintainability, we intend to refactor these synchronous calls and standardize the implementation of the API.
### Goals
- Try to avoid synchronous method calls in asynchronous methods.
- Async variable (AsyncResponse) is placed in the first parameter position.
- Async variable (AsyncResponse) cannot be substituted into method implementations.
- Add more tests and increase the coverage.
### Modification
1. Avoid synchronous method calls in asynchronous methods.
```
protected void internalDeleteNamespace(boolean authoritative) {
validateTenantOperation(namespaceName.getTenant(), TenantOperation.DELETE_NAMESPACE);
validatePoliciesReadOnlyAccess();
}
```
Suggest to do like this:
```
protected CompletableFuture<Void> internalDeleteNamespace(boolean authoritative) {
return validateTenantOperationAsync(namespaceName.getTenant(), TenantOperation.DELETE_NAMESPACE)
.thenCompose(__ -> validatePoliciesReadOnlyAccessAsync());
}
```
2. Async variable (AsyncResponse) is placed in the first parameter position
```
public void deleteNamespace(@Suspended final AsyncResponse asyncResponse,
@PathParam("tenant") String tenant,
@PathParam("namespace") String namespace,
@QueryParam("force") @DefaultValue("false") boolean force,
@QueryParam("authoritative") @DefaultValue("false") boolean authoritative) {
```
3. Async variable (AsyncResponse) cannot be substituted into method implementations
```
internalCreateNonPartitionedTopicAsync(asyncResponse, authoritative, properties);
```
Suggest to do like this:
```
internalCreateNonPartitionedTopicAsync(authoritative, properties)
.thenAccept(__ -> asyncResponse.resume(Response.noContent().build()))
.exceptionally(ex -> {
resumeAsyncResponseExceptionally(asyncResponse, ex.getCause());
return null;
});
```
### Track
In order to unify the modification and track the modified part, open an issue for the modified part first for tracking, like Apache/Pulsar#14013
--
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] mattisonchao commented on issue #14365: [PIP-142] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1067743341
Hi, @lhotari @Technoboy-
Is there any news in this PIP?
--
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] eolivelli commented on issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1048528890
I agree with this proposal and @codelipenghui 's proposal
One formality...
If this is a discussion it should stay on the mailing list.
This looks like a PIP doc.
I suggest to rename this to a PIP and let the discussion happen mostly on the dev@ list
--
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] lhotari commented on issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1044084575
> @codelipenghui @lhotari @eolivelli @BewareMyPower @nodece @mattisonchao I have opened this issue to discuss first then decide to make to PIP
Thank you @Technoboy- . Good work. I shared my thoughts and some expectations in yesterdays Pulsar Community meeting, you can find the discussion and @merlimat's responses of the rationale for the changes. It would be good to document the arguments that Matteo explained why mixing async and blocking (sync) causes issues.
Please check the last 35 minutes from the recording of the meeting (it's not available yet, @merlimat could you please add the recording on https://github.com/apache/pulsar/wiki/Community-Meetings#recordings ?)
I'll be able to follow up at the beginning of March the next time. There's no need to wait for me to reply.
While digging into the Jetty settings in Pulsar, I noticed a few gaps in backpressure handling, which are relevant when there are more requests which are handled asynchronously. I have a draft PR #14353 . I'll resume work on that in March. The current values for queue sizes and thread pool sizes are just guesses. Most likely we will use much lower values to prevent the broker taking in too much work in parallel. That's the essence of back pressure that it limits the in progress work so that incoming requests also slow down. Currently that is not the case since the thread pool queue can grow in an unbounded way (LinkedBlockingQueue is used under the covers). There are several kludges that attempt to add backpressure, but they aren't very effective in Pulsar currently. *#14353 will help address backpressure issues in Pulsar Admin API. These problems will come more evident when there are more APIs which are implemented using asynchronous Servlet API*. /cc @merlimat @codelipenghui
--
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- edited a comment on issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
Technoboy- edited a comment on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1043734052
@codelipenghui @lhotari @eolivelli @BewareMyPower @nodece @mattisonchao
I have opened this issue to discuss first then decide to make to PIP
--
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 issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1048487501
> Is it better to combine this one and #14353 want to do to one PIP such as PIP-142? WDYT? @Technoboy- @lhotari @merlimat
>
> The advantage is that we can provide a clearer image to the community that what we want to improve for the REST API part.
Yes, agree.
--
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 issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1043734052
@codelipenghui @lhotari @eolivelli @BewareMyPower @nodece @mattisonchao
I have open this issue to discuss first then decide to make to PIP
--
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 issue #14365: [DISCUSS] Standardize Admin REST API
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #14365:
URL: https://github.com/apache/pulsar/issues/14365#issuecomment-1048475738
Is it better to combine this one and https://github.com/apache/pulsar/pull/14353 want to do to one PIP such as PIP-142?
WDYT? @Technoboy- @lhotari @merlimat
The advantage is that we can provide a clearer image to the community that what we want to improve for the REST API 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