Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Fix uncompleted future when getting the topic policies of a deleted topic #18824

Merged
merged 1 commit into from
Dec 9, 2022
Merged

[fix][broker] Fix uncompleted future when getting the topic policies of a deleted topic #18824

merged 1 commit into from
Dec 9, 2022

Conversation

liangyepianzhou
Copy link
Contributor

@liangyepianzhou liangyepianzhou commented Dec 8, 2022

Motivation

Fix the uncompleted future when getting the topic policies of a deleted topic.

reader.readNextAsync().whenComplete((msg, e) -> {
if (e != null) {
future.completeExceptionally(e);
}
if (msg.getValue() != null
&& EventType.TOPIC_POLICY.equals(msg.getValue().getEventType())) {
TopicPoliciesEvent topicPoliciesEvent = msg.getValue().getTopicPoliciesEvent();
if (topicName.equals(TopicName.get(
topicPoliciesEvent.getDomain(),
topicPoliciesEvent.getTenant(),
topicPoliciesEvent.getNamespace(),
topicPoliciesEvent.getTopic()))
) {
fetchTopicPoliciesAsyncAndCloseReader(reader, topicName,
topicPoliciesEvent.getPolicies(), future);
} else {
fetchTopicPoliciesAsyncAndCloseReader(reader, topicName, policies, future);
}
}
});
} else {
future.complete(policies);
reader.closeAsync().whenComplete((v, e) -> {

Modification

future.complete(null); when msg.getValue() == null.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: liangyepianzhou#17

… deleted topic

### Motivation
Fix uncompleted future when get the topic policies of a deleted topic.
### Modification
`future.complete(null);` when `msg.getValue() != null`.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 8, 2022
@AnonHxy
Copy link
Contributor

AnonHxy commented Dec 9, 2022

LGTM. It seems that this method will only be called from UT test case now

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2022

Codecov Report

Merging #18824 (3b54a05) into master (cd85a67) will increase coverage by 0.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18824      +/-   ##
============================================
+ Coverage     47.39%   48.12%   +0.72%     
- Complexity    10479    10730     +251     
============================================
  Files           698      703       +5     
  Lines         68070    68819     +749     
  Branches       7279     7376      +97     
============================================
+ Hits          32264    33118     +854     
+ Misses        32228    32003     -225     
- Partials       3578     3698     +120     
Flag Coverage Δ
unittests 48.12% <100.00%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../service/SystemTopicBasedTopicPoliciesService.java 72.25% <100.00%> (+7.53%) ⬆️
.../transaction/buffer/metadata/AbortTxnMetadata.java 28.57% <0.00%> (-57.15%) ⬇️
...ker/service/schema/exceptions/SchemaException.java 60.00% <0.00%> (-40.00%) ⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 60.00% <0.00%> (-20.00%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 42.19% <0.00%> (-15.44%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 30.32% <0.00%> (-12.81%) ⬇️
.../org/apache/pulsar/broker/service/PulsarStats.java 72.56% <0.00%> (-7.97%) ⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java 78.04% <0.00%> (-7.32%) ⬇️
...apache/pulsar/client/impl/AutoClusterFailover.java 70.00% <0.00%> (-6.12%) ⬇️
... and 114 more

@liangyepianzhou liangyepianzhou merged commit b311961 into apache:master Dec 9, 2022
congbobo184 pushed a commit that referenced this pull request Dec 9, 2022
liangyepianzhou added a commit that referenced this pull request Dec 9, 2022
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 10, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
… deleted topic (apache#18824)

Fix the uncompleted future when getting the topic policies of a deleted topic.
https://github.com/apache/pulsar/blob/30b52a1ac11b4be485258140a167b5e635586a36/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L513-L535
`future.complete(null);` when `msg.getValue() == null`.

(cherry picked from commit b311961)
(cherry picked from commit 1d98196)
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
… deleted topic (apache#18824)

Fix the uncompleted future when getting the topic policies of a deleted topic.
https://github.com/apache/pulsar/blob/30b52a1ac11b4be485258140a167b5e635586a36/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L513-L535
`future.complete(null);` when `msg.getValue() == null`.

(cherry picked from commit b311961)
(cherry picked from commit 1d98196)
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
@liangyepianzhou liangyepianzhou deleted the xiangying/fix/topic/fix_uncompleteFuture_of_get_topic_policy branch February 27, 2023 03:27
@momo-jun momo-jun changed the title [fix][broker] Fix uncompleted future when get the topic policies of a deleted topic [fix][broker] Fix uncompleted future when getting the topic policies of a deleted topic Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants