Skip to content

[improve] [broker] Pin AppendIndexMetadataInterceptor to field in ManagedLedgerInterceptorImpl - #20112

Merged
Technoboy- merged 3 commits into
apache:masterfrom
lifepuzzlefun:pin_append_index_metadata_interceptor
Apr 20, 2023
Merged

Technoboy- merged 3 commits into
apache:masterfrom
lifepuzzlefun:pin_append_index_metadata_interceptor

Conversation

@lifepuzzlefun

@lifepuzzlefun lifepuzzlefun commented Apr 15, 2023 •

Copy link
Copy Markdown
Contributor

Motivation

most method in ManagedLedgerInterceptorImpl is just iterate all the interceptors to find the AppendIndexMetadataInterceptor which is unnessary.

Modifications

just save AppendIndexMetadataInterceptor to field when needed just call interceptor directly.

Verifying this change

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

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
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 15, 2023
@AnonHxy

AnonHxy commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

LGTM

@BewareMyPower BewareMyPower added this to the 3.1.0 milestone Apr 17, 2023
lifepuzzlefun and others added 3 commits April 20, 2023 13:53
…torImpl` to avoid iterate interceptors only to find AppendIndexMetadataInterceptor
…/ManagedLedgerInterceptorImpl.java

Co-authored-by: Nicolò Boschi <[email protected]>
@Technoboy-
Technoboy- force-pushed the pin_append_index_metadata_interceptor branch from 943a82b to 1d6de86 Compare April 20, 2023 05:53
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #20112 (1d6de86) into master (9b72302) will increase coverage by 39.74%.
The diff coverage is 82.66%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20112       +/-   ##
=============================================
+ Coverage     33.17%   72.92%   +39.74%     
- Complexity    12236    31928    +19692     
=============================================
  Files          1499     1868      +369     
  Lines        114413   138413    +24000     
  Branches      12431    15233     +2802     
=============================================
+ Hits          37962   100937    +62975     
+ Misses        71499    29440    -42059     
- Partials       4952     8036     +3084     
Flag Coverage Δ
inttests 24.20% <5.33%> (?)
systests 24.78% <6.66%> (?)
unittests 72.22% <82.66%> (+39.04%) ⬆️

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

Impacted Files Coverage Δ
...e/pulsar/broker/authentication/oidc/JwksCache.java 56.62% <0.00%> (ø)
...thentication/oidc/OpenIDProviderMetadataCache.java 78.48% <0.00%> (ø)
...unctions/runtime/kubernetes/KubernetesRuntime.java 38.34% <0.00%> (+38.34%) ⬆️
...broker/intercept/ManagedLedgerInterceptorImpl.java 75.00% <75.00%> (+75.00%) ⬆️
.../pulsar/broker/delayed/bucket/ImmutableBucket.java 88.49% <76.19%> (+88.49%) ⬆️
...elayed/bucket/BookkeeperBucketSnapshotStorage.java 83.63% <100.00%> (+83.63%) ⬆️
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 84.21% <100.00%> (+84.21%) ⬆️
...he/pulsar/broker/delayed/bucket/MutableBucket.java 93.26% <100.00%> (+93.26%) ⬆️
...ava/org/apache/pulsar/broker/service/Consumer.java 86.64% <100.00%> (+26.85%) ⬆️
...ava/org/apache/pulsar/broker/service/Producer.java 82.46% <100.00%> (+25.20%) ⬆️

... and 1531 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants