Skip to content

[fix][broker] Fix get topic policies as null during clean cache#20763

Merged
codelipenghui merged 4 commits intoapache:masterfrom
coderzc:fix_topic_policy_null
Jul 12, 2023
Merged

[fix][broker] Fix get topic policies as null during clean cache#20763
codelipenghui merged 4 commits intoapache:masterfrom
coderzc:fix_topic_policy_null

Conversation

@coderzc
Copy link
Copy Markdown
Member

@coderzc coderzc commented Jul 9, 2023

Motivation

Currently, we may get topic policies as null during the clean topic policies since there is a race condition between them.

time thread1 getTopicPolicies thread2 cleanCacheAndCloseReader
t1 check if policyCacheInitMap exists
the value of this namespace
remove the reader of this namespace from readerCaches
t2 the value of this namespace in the
policyCacheInitMap is true
remove the policies of this namespace from policiesCache
t3 gets the topicPolicies of this namespace
from policiesCache as null
t4 remove the value of this namespace from policyCacheInitMap

Modifications

Use policyCacheInitMap.compute() to make get topic policies init flag and clean cache mutually exclusive.

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

Documentation

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

Matching PR in forked repository

PR in forked repository:

@coderzc coderzc self-assigned this Jul 9, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jul 9, 2023
@coderzc coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker and removed doc-not-needed Your PR changes do not impact docs labels Jul 9, 2023
@coderzc coderzc added this to the 3.1.0 milestone Jul 9, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jul 9, 2023
@coderzc coderzc requested a review from mattisonchao July 10, 2023 02:14
@coderzc coderzc force-pushed the fix_topic_policy_null branch 3 times, most recently from 64d1f54 to 7d39b82 Compare July 10, 2023 04:29
@coderzc coderzc force-pushed the fix_topic_policy_null branch from 7d39b82 to 295ad12 Compare July 10, 2023 04:32
}

@Test
public void testGetTopicPoliciesWithCleanCache()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderzc Can you reproduce the issue with this test with the fix? I have tried it on my laptop without this fix. But can't reproduce the issue. The fix looks good, just the test can't protect the changes of this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rewritten the test to make it can reproduce the issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

});
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you delete the blank line?

@coderzc coderzc closed this Jul 12, 2023
@coderzc coderzc reopened this Jul 12, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 12, 2023

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.11%. Comparing base (0bac873) to head (eae2a70).
⚠️ Report is 1917 commits behind head on master.

Files with missing lines Patch % Lines
.../service/SystemTopicBasedTopicPoliciesService.java 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20763       +/-   ##
=============================================
+ Coverage     36.80%   73.11%   +36.31%     
- Complexity    12073    32128    +20055     
=============================================
  Files          1689     1866      +177     
  Lines        129387   139041     +9654     
  Branches      14120    15294     +1174     
=============================================
+ Hits          47619   101661    +54042     
+ Misses        75482    29322    -46160     
- Partials       6286     8058     +1772     
Flag Coverage Δ
inttests 24.17% <42.85%> (+0.08%) ⬆️
systests 25.01% <21.42%> (-0.11%) ⬇️
unittests 72.41% <92.85%> (+40.49%) ⬆️

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

Files with missing lines Coverage Δ
.../service/SystemTopicBasedTopicPoliciesService.java 76.96% <92.85%> (+18.93%) ⬆️

... and 1431 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderzc added a commit that referenced this pull request Aug 22, 2023
@coderzc coderzc added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Aug 22, 2023
coderzc added a commit that referenced this pull request Aug 28, 2023
touchkey pushed a commit to sajithsebastian/pulsar that referenced this pull request Aug 28, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
@coderzc coderzc deleted the fix_topic_policy_null branch August 29, 2023 08:52
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 13, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
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.

5 participants