Skip to content

Conversation

@AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Oct 29, 2023

Motivation

Fix the resource_quota_zpath in LoadSimulationController and ModularLoadManagerImpl.

As we can see from the BundlesQuotas(line30) that the resource_quota_zpath should be /loadbalance/resource-quota, not /loadbalance/resource-quota/namespace. so here we fix it.

private static final String RESOURCE_QUOTA_ROOT = "/loadbalance/resource-quota";
private static final String DEFAULT_RESOURCE_QUOTA_PATH = RESOURCE_QUOTA_ROOT + "/default";

Modifications

Fix resource_quota_zpath

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

Matching PR in forked repository

AnonHxy#47

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 29, 2023
@AnonHxy AnonHxy self-assigned this Oct 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.24%. Comparing base (e55de39) to head (125ffd0).
Report is 1415 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21461      +/-   ##
============================================
+ Coverage     73.14%   73.24%   +0.09%     
- Complexity    32570    32681     +111     
============================================
  Files          1890     1892       +2     
  Lines        140357   140707     +350     
  Branches      15425    15483      +58     
============================================
+ Hits         102663   103054     +391     
+ Misses        29595    29541      -54     
- Partials       8099     8112      +13     
Flag Coverage Δ
inttests 24.09% <ø> (?)
systests 24.66% <ø> (-0.06%) ⬇️
unittests 72.53% <ø> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...roker/loadbalance/impl/ModularLoadManagerImpl.java 84.21% <ø> (+2.30%) ⬆️

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

@liudezhi2098
Copy link
Contributor

This will lead to incompatibility between versions.

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Oct 31, 2023

Hi @liudezhi2098 . This patch will not lead incompatible issue I think:

  1. Resource quotas are written to /loadbalance/resource-quota only by BundlesQuotas.

public CompletableFuture<Void> setResourceQuota(String bundle, ResourceQuota quota) {
return resourceQuotaCache.readModifyUpdateOrCreate(RESOURCE_QUOTA_ROOT + "/" + bundle,
__ -> quota)
.thenApply(__ -> null);
}

  1. ModularLoadManagerImpl only read the zpath specified by RESOURCE_QUOTA_ZPATH=/loadbalance/resource-quota/namespace. Currently the read result should always be empty because the zpath is wrong.

Optional<ResourceQuota> optQuota = resourceQuotaCache
.get(String.format("%s/%s", RESOURCE_QUOTA_ZPATH, bundle)).join();
if (optQuota.isPresent()) {
ResourceQuota quota = optQuota.get();

  1. And the LoadSimulationController should be used for test. We needn't care about it's incompatibility

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Nov 6, 2023

@HQebupt @congbobo184 @Technoboy- @codelipenghui PTAL. thanks:)

@Technoboy- Technoboy- added this to the 3.2.0 milestone Nov 6, 2023
Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

We'd better make BundlesQuotas#DEFAULT_RESOURCE_QUOTA_PATH to public, and remove them from ModularLoadManagerImpl/LoadSimulationController

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Nov 6, 2023

We'd better make BundlesQuotas#DEFAULT_RESOURCE_QUOTA_PATH to public, and remove them from ModularLoadManagerImpl/LoadSimulationController

@Technoboy- Good idea. PIP-301 #21129 is going to refactor this and will make it public. I think we could just correct the zpath here and make it to public in another PR when implements PIP-301

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Is there any test that could cover it?

@codelipenghui
Copy link
Contributor

@AnonHxy Could you please help add a test to cover this fix? And from the description, I still don't know what is the impact here. Is it something that doesn't work? how to reproduce it?

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Nov 10, 2023

Hi @codelipenghui @poorbarcode @BewareMyPower Is there any other comments:)

public void testBundleDataDefaultValue() throws Exception {
final String tenant = "test";
final String cluster = "test";
String namespace = tenant + "/" + cluster + "/" + "test";
Copy link
Contributor

@poorbarcode poorbarcode Nov 15, 2023

Choose a reason for hiding this comment

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

This test sets the default bundle resource quota by Pulsar Admin V1 API, could you also add a case for Pulsar Admin V2 API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

8 participants