Skip to content

[fix][offload] fix offload metrics error#20366

Merged
codelipenghui merged 7 commits into
apache:masterfrom
ethqunzhong:qunzhong-fix-offload-metrics
May 31, 2023
Merged

[fix][offload] fix offload metrics error#20366
codelipenghui merged 7 commits into
apache:masterfrom
ethqunzhong:qunzhong-fix-offload-metrics

Conversation

@ethqunzhong

@ethqunzhong ethqunzhong commented May 22, 2023

Copy link
Copy Markdown
Contributor

Related issue #20355

Motivation

The current implementation has the following issues:

  1. When constructing offload metrics, using the managedLeger name as topicName results in an incorrect format, causing abnormal display of metrics on Grafana.
  2. When calculating offload bytes for Filesystem offload, it incorrectly calculates the actual write value, resulting in much larger metrics than the actual value.

Modifications

  1. Add the fromPersistenceNamingEncoding method to TopicName and use mlname to obtain the topic name.
  2. Replace the part of using mlname in the logic of topic label before offload metrics with using topicname.
  3. Modify the logic of recordOffloadBytes in filesystem offloader, change entry.getLength() to entry.getEntryBytes().length.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing 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: ethqunzhong#6

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 22, 2023
eolivelli
eolivelli approved these changes May 22, 2023
View reviewed changes

@eolivelli eolivelli left a comment

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.

Great catch

@dlg99 PTAL

@ethqunzhong

ethqunzhong commented May 22, 2023

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter

codecov-commenter commented May 22, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #20366 (97854e4) into master (426ad3e) will decrease coverage by 0.04%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20366      +/-   ##
============================================
- Coverage     72.93%   72.89%   -0.04%     
+ Complexity    31973    31890      -83     
============================================
  Files          1868     1866       -2     
  Lines        138663   138493     -170     
  Branches      15248    15194      -54     
============================================
- Hits         101133   100961     -172     
- Misses        29484    29510      +26     
+ Partials       8046     8022      -24
Flag Coverage Δ
inttests 24.07% <0.00%> (-0.04%) ⬇️
systests 24.94% <38.88%> (+0.07%) ⬆️
unittests 72.17% <84.61%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java 43.65% <ø> (ø)
...ker/loadbalance/impl/LinuxBrokerHostUsageImpl.java 79.68% <ø> (-4.69%) ⬇️
...d/jcloud/impl/BlobStoreManagedLedgerOffloader.java 75.06% <33.33%> (-0.32%) ⬇️
...esystem/impl/FileSystemManagedLedgerOffloader.java 68.04% <75.00%> (+0.84%) ⬆️
...filesystem/impl/FileStoreBackedReadHandleImpl.java 56.47% <80.00%> (+0.51%) ⬆️
...ava/org/apache/pulsar/common/naming/TopicName.java 96.85% <100.00%> (+0.52%) ⬆️
...ad/jcloud/impl/BlobStoreBackedInputStreamImpl.java 97.40% <100.00%> (+0.03%) ⬆️
...oad/jcloud/impl/BlobStoreBackedReadHandleImpl.java 80.14% <100.00%> (+0.14%) ⬆️
...d/jcloud/impl/BlobStoreBackedReadHandleImplV2.java 61.14% <100.00%> (+0.24%) ⬆️
.../jcloud/impl/BlockAwareSegmentInputStreamImpl.java 90.00% <100.00%> (+0.06%) ⬆️

... and 102 files with indirect coverage changes

@ethqunzhong

ethqunzhong commented May 23, 2023

Copy link
Copy Markdown
Contributor Author

@eolivelli @dlg99 all test passed. PTAL

@ethqunzhong

ethqunzhong commented May 25, 2023

Copy link
Copy Markdown
Contributor Author

@tjiuming PTAL

lhotari
lhotari requested changes May 29, 2023
View reviewed changes
Comment thread .../org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java Outdated
@ethqunzhong ethqunzhong requested a review from lhotari May 29, 2023 11:43
lhotari
lhotari approved these changes May 30, 2023
View reviewed changes

@lhotari lhotari left a comment

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.

Good work @ethqunzhong

@poorbarcode

poorbarcode commented May 30, 2023

Copy link
Copy Markdown
Contributor

@hangc0276 Could you take a look?

@poorbarcode poorbarcode added this to the 3.1.0 milestone May 30, 2023
hangc0276
hangc0276 approved these changes May 30, 2023
View reviewed changes

@hangc0276 hangc0276 left a comment

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.

Overall LGTM. @asafm Please help take a look at this PR, thanks.

Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
@ethqunzhong

ethqunzhong commented May 31, 2023

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@codelipenghui codelipenghui merged commit 8e0ebba into apache:master May 31, 2023
RobertIndie pushed a commit that referenced this pull request Jun 7, 2023
@ethqunzhong @RobertIndie
[fix][offload] fix offload metrics error (#20366)
223dbf4 
(cherry picked from commit 8e0ebba)
Technoboy- pushed a commit that referenced this pull request Jun 15, 2023
@ethqunzhong @Technoboy-
[fix][offload] fix offload metrics error (#20366)
31907c4
@tjiuming

tjiuming commented Jun 30, 2023

Copy link
Copy Markdown
Contributor

good catch!

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

Reviewers

@lhotari lhotari lhotari approved these changes

@hangc0276 hangc0276 hangc0276 approved these changes

@dlg99 dlg99 dlg99 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@aloyszhang aloyszhang Awaiting requested review from aloyszhang

@zymap zymap Awaiting requested review from zymap

Assignees

@ethqunzhong ethqunzhong

Labels

cherry-picked/branch-2.11 cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs release/2.11.2 release/3.0.1

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.