Skip to content
This repository has been archived by the owner on Apr 1, 2024. It is now read-only.

ISSUE-14365: [PIP-149] Standardize Admin REST API #3788

Open
sijie opened this issue Feb 18, 2022 · 1 comment
Open

ISSUE-14365: [PIP-149] Standardize Admin REST API #3788

sijie opened this issue Feb 18, 2022 · 1 comment

Comments

@sijie
Copy link
Member

sijie commented Feb 18, 2022

Original Issue: apache#14365


Co-author: @mattisonchao @Technoboy-

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 called synchronous implementations. Also, many synchronous calls do not add timeouts. This greatly reduces concurrency, user operations, and experience.
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.

Related discussion: https://lists.apache.org/thread/pkkz2jgwtzpksp6d4rdm1pyxzb3z6vmg

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;
            });
    
    

Task tracking

In order to unify the modification and track the modified part, it's better to open an issue to track, like apache#14353, apache#14013, apache#13854.

@github-actions
Copy link

The issue had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Mar 21, 2022
@sijie sijie changed the title ISSUE-14365: [DISCUSS] Standardize Admin REST API ISSUE-14365: [PIP-149] Standardize Admin REST API Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant