[Issue 13854][broker] Fix call sync method in async rest api for internalTruncateNonPartitionedTopic #13904

HQebupt · 2022-01-22T08:57:14Z

Master Issue: #13854

Motivation

See #13854

Verifying this change

Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

no-need-doc

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

Jason918 · 2022-01-25T12:49:46Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

HQebupt · 2022-01-28T13:22:34Z

/pulsarbot run-failure-checks

HQebupt · 2022-01-29T02:29:45Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

HQebupt · 2022-01-29T07:58:02Z

@Jason918 @Technoboy- @liudezhi2098 @mattisonchao Pls take a look.

HQebupt · 2022-01-29T08:43:24Z

/pulsarbot run-failure-checks

HQebupt · 2022-01-30T13:34:37Z

/pulsarbot run-failure-checks

Jason918 · 2022-02-08T04:07:15Z

/pulsarbot run-failure-checks

mattisonchao

I left some suggestion here, PTAL :-)

mattisonchao · 2022-02-08T03:25:56Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+                .thenCompose(__ -> getTopicReferenceAsync(topicName))
+                .thenCompose(topic -> topic.truncate())
+                .thenAccept(__ -> {
+                    asyncResponse.resume(new RestException(Response.Status.NO_CONTENT.getStatusCode(),


Why resume Exception ?

mattisonchao · 2022-02-08T03:27:16Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+            getPartitionedTopicMetadataAsync(topicName, authoritative, false)
+                    .whenComplete((meta, t) -> {
+                        if (meta.partitions > 0) {
+                            final List<CompletableFuture<Void>> futures = Lists.newArrayList();


Suggested change

final List<CompletableFuture<Void>> futures = Lists.newArrayList();

final List<CompletableFuture<Void>> futures = Lists.newArrayList(meta.partitions);

mattisonchao · 2022-02-08T03:27:49Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+                                try {
+                                    futures.add(pulsar().getAdminClient().topics()
+                                            .truncateAsync(topicNamePartition.toString()));
+                                } catch (Exception e) {


Suggested change

} catch (Exception e) {

} catch (PulsarServerException e) {

mattisonchao · 2022-02-08T03:29:14Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+                                    futures.add(pulsar().getAdminClient().topics()
+                                            .truncateAsync(topicNamePartition.toString()));
+                                } catch (Exception e) {
+                                    log.error("[{}] Failed to truncate topic {}", clientAppId(), topicNamePartition, e);


Suggested change

log.error("[{}] Failed to truncate topic {}", clientAppId(), topicNamePartition, e);

log.error("[{}] Failed to truncate topic, while getting admin client {}", clientAppId(), topicNamePartition, e);

mattisonchao · 2022-02-08T05:08:42Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

                            }
+                            FutureUtil.waitForAll(futures).handle((result, exception) -> {


Use whenComplete instead of handle or return this future to the upper layer future?

mattisonchao · 2022-02-08T05:14:29Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

+                                                    "Proxy not authorized to access resource (proxy:%s,original:%s)",
+                                                    clientAppId, originalPrincipal));
+                                }
+                                return null;


mattisonchao · 2022-02-08T05:16:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

+                                            originalPrincipal, clientAppId, tenant);
+                                    future.complete(null);
+                                }
+                                return null;


mattisonchao · 2022-02-08T05:17:24Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

+                                            log.debug("Successfully authorized {} on tenant {}", clientAppId, tenant);
+                                            future.complete(null);
+                                        }
+                                        return null;


mattisonchao · 2022-02-08T05:18:45Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

+                                    });
+                        }
+                    }
+                    return null;


mattisonchao · 2022-02-08T05:21:37Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

+            PulsarService pulsar, String clientAppId,
+            String originalPrincipal, String tenant,
+            AuthenticationDataSource authenticationData) {
+        CompletableFuture<Void> future = new CompletableFuture<>();


Could we use combine or another method to avoid using global future that complete at a lot of methods? it's hard to read the code. I think.

github-actions · 2022-03-19T01:56:07Z

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

…onedTopic

github-actions · 2022-05-28T02:12:19Z

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

github-actions bot assigned HQebupt Jan 22, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 22, 2022

Jason918 requested changes Jan 22, 2022

View reviewed changes

Jason918 added release/2.8.3 release/2.9.3 labels Jan 22, 2022

Jason918 mentioned this pull request Jan 22, 2022

[Broker] PersistentTopicsBase has many async method call sync method. #13854

Closed

26 tasks

codelipenghui added this to the 2.10.0 milestone Jan 24, 2022

HQebupt force-pushed the issue-13854-internalTruncateNonPartitionedTopic branch from 3c8ee2b to 612d1d4 Compare January 25, 2022 02:32

Jason918 reviewed Jan 25, 2022

View reviewed changes

Jason918 reviewed Jan 29, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java Outdated Show resolved Hide resolved

Jason918 approved these changes Jan 29, 2022

View reviewed changes

Jason918 requested review from codelipenghui and Technoboy- January 29, 2022 08:28

This was referenced Feb 7, 2022

[Broker]make revokePermissionsOnTopic method async #14149

Merged

[Broker]make grantPermissionsOnTopic method async #14152

Merged

liudezhi2098 approved these changes Feb 8, 2022

View reviewed changes

mattisonchao reviewed Feb 8, 2022

View reviewed changes

codelipenghui modified the milestones: 2.10.0, 2.11.0 Feb 8, 2022

michaeljmarshall added release/2.8.4 and removed release/2.8.3 labels Feb 11, 2022

codelipenghui removed release/2.9.3 release/2.8.4 labels Feb 16, 2022

github-actions bot added the lifecycle/stale label Mar 19, 2022

Fix call sync method in async rest api for internalTruncateNonPartiti…

f7065f6

…onedTopic

HQebupt force-pushed the issue-13854-internalTruncateNonPartitionedTopic branch from d65be28 to f7065f6 Compare March 21, 2022 15:38

github-actions bot added the Stale label May 28, 2022

HQebupt closed this Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 13854][broker] Fix call sync method in async rest api for internalTruncateNonPartitionedTopic #13904

[Issue 13854][broker] Fix call sync method in async rest api for internalTruncateNonPartitionedTopic #13904

HQebupt commented Jan 22, 2022

Jason918 commented Jan 25, 2022

HQebupt commented Jan 28, 2022

HQebupt commented Jan 29, 2022

HQebupt commented Jan 29, 2022 •

edited

Loading

HQebupt commented Jan 29, 2022

HQebupt commented Jan 30, 2022

Jason918 commented Feb 8, 2022

mattisonchao left a comment

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

mattisonchao Feb 8, 2022

github-actions bot commented Mar 19, 2022

github-actions bot commented May 28, 2022

	final List<CompletableFuture<Void>> futures = Lists.newArrayList();
	final List<CompletableFuture<Void>> futures = Lists.newArrayList(meta.partitions);

	log.error("[{}] Failed to truncate topic {}", clientAppId(), topicNamePartition, e);
	log.error("[{}] Failed to truncate topic, while getting admin client {}", clientAppId(), topicNamePartition, e);

		}
		FutureUtil.waitForAll(futures).handle((result, exception) -> {

[Issue 13854][broker] Fix call sync method in async rest api for internalTruncateNonPartitionedTopic #13904

[Issue 13854][broker] Fix call sync method in async rest api for internalTruncateNonPartitionedTopic #13904

Conversation

HQebupt commented Jan 22, 2022

Motivation

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Jason918 commented Jan 25, 2022

HQebupt commented Jan 28, 2022

HQebupt commented Jan 29, 2022

HQebupt commented Jan 29, 2022 • edited Loading

HQebupt commented Jan 29, 2022

HQebupt commented Jan 30, 2022

Jason918 commented Feb 8, 2022

mattisonchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2022

github-actions bot commented May 28, 2022

HQebupt commented Jan 29, 2022 •

edited

Loading