Skip to content

[improve][admin] Expose the offload threshold in seconds to the admin#22101

Merged
Technoboy- merged 1 commit intoapache:masterfrom
zymap:expose-threshold-in-seconds-to-admin
Feb 23, 2024
Merged

[improve][admin] Expose the offload threshold in seconds to the admin#22101
Technoboy- merged 1 commit intoapache:masterfrom
zymap:expose-threshold-in-seconds-to-admin

Conversation

@zymap
Copy link
Copy Markdown
Member

@zymap zymap commented Feb 23, 2024


Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

We have two configurations to trigger the offload automatically, time and bytes. We only expose the bytes in the admin but didn't expose the time in the admin. When we want to disable it, we need to set both of them to the -1. But we didn't expose the time settings, the offload will never be disabled.

Modifications

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

Documentation

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

Matching PR in forked repository

PR in forked repository:

---

### Motivation

We have two configurations to trigger the offload automatically,
time and bytes. We only expose the bytes in the admin but didn't
expose the time in the admin. When we want to disable it, we need
to set both of them to the -1. But we didn't expose the time settings,
the offload will never be disabled.
@github-actions
Copy link
Copy Markdown

@zymap Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@Technoboy- Technoboy- added ready-to-test doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Feb 23, 2024
@Technoboy- Technoboy- closed this Feb 23, 2024
@Technoboy- Technoboy- reopened this Feb 23, 2024
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 23, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.58%. Comparing base (5a614e9) to head (c76bb1e).
⚠️ Report is 1253 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #22101       +/-   ##
=============================================
+ Coverage     37.02%   73.58%   +36.55%     
- Complexity    12201    32592    +20391     
=============================================
  Files          1734     1874      +140     
  Lines        132277   139354     +7077     
  Branches      14480    15279      +799     
=============================================
+ Hits          48974   102537    +53563     
+ Misses        76823    28909    -47914     
- Partials       6480     7908     +1428     
Flag Coverage Δ
inttests 24.61% <0.00%> (-0.20%) ⬇️
systests 24.38% <0.00%> (-0.05%) ⬇️
unittests 72.84% <100.00%> (+40.86%) ⬆️

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

Files with missing lines Coverage Δ
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java 78.73% <100.00%> (+78.73%) ⬆️

... and 1440 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.

@heesung-sohn
Copy link
Copy Markdown
Contributor

This PR depends on TimeUnitToSecondsConverter, which has not been cherry-picked to branch-3.0.

#20782

@heesung-sohn
Copy link
Copy Markdown
Contributor

We are reverting this commit from branch-3.0.

@heesung-sohn
Copy link
Copy Markdown
Contributor

heesung-sohn commented Feb 29, 2024

@zymap Plz raise another PR to branch-3.0 without relying on TimeUnitToSecondsConverter(because TimeUnitToSecondsConverter is not available in branch-3.0), if possible. (targeting release 3.0.4)

zymap added a commit to zymap/pulsar that referenced this pull request Mar 1, 2024
@zymap
Copy link
Copy Markdown
Member Author

zymap commented Mar 1, 2024

#22169 a new PR to branch-3.0 @heesung-sn

mukesh154 pushed a commit to cognitree/pulsar that referenced this pull request Mar 1, 2024
@heesung-sohn heesung-sohn changed the title [improve][admin] Expose the offload threshold in seconds to the amdin [improve][admin] Expose the offload threshold in seconds to the admin Mar 1, 2024
gaoran10 added a commit that referenced this pull request Mar 4, 2024
bahetimansi pushed a commit to bahetimansi/pulsar that referenced this pull request Mar 23, 2025
…he amdin (apache#22101)"

This reverts commit 6e988ef.

(cherry picked from commit ca35b2e)
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.

7 participants