Fix resource group keep setting incorrect softMemoryLimit

[brandboat]

Description

When using database resource group manager, if softMemoryLimit was setup to 80% and then modified to 100MB. The softMemoryLimit in resource group is always 80% instead of 100MB.
The root cause is after using fraction(i.e. percentage) in softMemoryLimit in resource group, SqlQueryManager start updating softMemoryLimit to resource group per second. And the background update will not stop even if a static value is set in softMemoryLimit.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# General
* Fix resource groups not noticing updated `softMemoryLimit` if changed from a percent-based value to exact value. ({issue}`issuenumber`)

[brandboat]

https://github.com/trinodb/trino/actions/runs/6747796781/job/18345256146?pr=19626
failed test TestMinWorkerRequirement.testMultipleRequiredWorkerNodesSessionOverride in ci is flaky #19446

[hashhar]

we should also assert that the initial change to percent based value also took effect using an assert similar to this.

[brandboat]

we should also assert that the initial change to percent based value also took effect using an assert similar to this.

Not sure if there is any better way to do assertion, but I found this might cause flaky tests. (the Math.round part)

        ResourceGroupInfo groupInfo =
                manager.tryGetResourceGroupInfo(new ResourceGroupId(new ResourceGroupId("global"), "bi-user"))
                        .orElseThrow(() -> new IllegalStateException("Resource group not found"));
        assertEquals(
                groupInfo.getSoftMemoryLimit(),
                DataSize.of(Math.round(queryRunner.getCoordinator().getClusterMemoryManager().getClusterMemoryBytes() * 0.2), BYTE));

(updated) : make softMemoryLimit percentage to 100% to avoid Round-off Error

[sopel39]

(updated) : make softMemoryLimit percentage to 100% to avoid Round-off Error

You could have used org.testng.Assert#assertEquals(double, double, double) (with delta) to avoid exact match

[brandboat]

Thanks for the tips !

[hashhar]

is this "required" for the test? AFAIK we only care about switching from fraction based to exact value so whatever was used before fraction based doesn't seem to matter.

[brandboat]

Many thanks for the comments !

is this "required" for the test?

We don't need that actually, will get rid of this line.

[hashhar]

LGTM

@losipiuk / @sopel39 can you PTAL as well since I'm not the expert here.

[losipiuk]

nit: I would prefer to do active wait with shorter sleeps, to finish test as quickly as possible. But given other tests use long sleeps already you can leave it as is. Test methods can be cleaned up in batch.

[brandboat]

I would prefer to do active wait with shorter sleeps, to finish test as quickly as possible.

Did you mean MILLISECONDS.sleep(1000); ? If so, I can change that in this pr. While I won't touch other test methods as they should be take care in a separate pr.

[sopel39]

Did you mean MILLISECONDS.sleep(1000); ? If so, I can change that in this pr. While I won't touch other test methods as they should be take care in a separate pr.

This has a risk of becoming flaky then, right? I agree with @losipiuk that active loop with shorter sleeps would be more reliable.

[brandboat]

This has a risk of becoming flaky then, right? I agree with losipiuk that active loop with shorter sleeps would be more reliable.

Sorry about misunderstanding... did you mean something like

for (int i = 0; i < 2000; i++) {
  MILLISECONDS.sleep(1);
}

[losipiuk]

You can use assertEventually

        assertEventually(
                new Duration(2, TimeUnit.SECONDS),
                new Duration(100, TimeUnit.MILLISECONDS),
                () -> {.... code which does assertions});

[losipiuk]

Also not an expert but LGTM