NoSQL: reduce heap pressure when running tests#3267
Conversation
Some tests generate a lot of realms, likely one realm per test case. While the amount of data per realm is not much, it is nontheless nice to remove that data immediately (for tests). The maintenance service, which purges data of eligible realms, cannot be run against the in-memory backend (different JVM). This change adds a rather "test only" workaround to purge the realm data in the in-memory backend immediately.
|
|
||
| if (updated.status() == PURGED) { | ||
| realmManagement.delete(updated); | ||
| if (updated.status() == PURGING || updated.status() == PURGED) { |
There was a problem hiding this comment.
Do we really need the complex state transition sequence (via INACTIVE) with a loop now? Why not transition to the final state in one step?
There was a problem hiding this comment.
Realm-managment defines possible realm-states and allowed transitions between those.
The intent, in this "purge realm" case, is to later allow a workflow to first set a realm to INACTIVE (aka it's still there, but you cannot use it), allowing users to revert their decision within a couple of days, pretty much what people can do in other SaaS offerings. From INACTIVE it can transition to PURGING, which means that the management-service takes care of actually deleting all realm data and eventually delete the realm definition itself.
There was a problem hiding this comment.
Yes, I understand the current state transitions :) however, since in this piece of code the intent is to transition from any non-purged state to PURGING, having state transition restrictions in RealmManagementImpl seems artificial. This code will transition to the state it wants anyway, so why impede this transition and make it go through a loop?
This is a tangential comment based on general observations... not a blocker for this PR :)
| if (result.status() == PURGING && "InMemory".equals(backend.type())) { | ||
| // Handle the test/development case when using the in-memory backend: | ||
| // In this case there is no chance to run the maintenance service. To remove no longer | ||
| // necessary data from the Java heap, call the in-memory backend directly. | ||
| // This is "nice behavior" when running tests. | ||
| // The only "cost" is that the state PURGING may never be pushed to PURGED, but that is | ||
| // acceptable. | ||
| backend.deleteRealms(Set.of(realmId)); |
There was a problem hiding this comment.
This looks awkward... Could we delegate that to Backend impl?... or maybe move the special case to PolarisIntegrationTestFixture?
There was a problem hiding this comment.
It is a bit awkward, true. OTOH, InMemory is the only backend that doesn't persist anything, so we cannot use the management-service or even the admin-tool here.
PolarisIntegrationTestFixture can't be used in all tests, as it's only for Quarkus tests.
This change actually also applies when people try Polaris server w/ the InMemory backend.
There was a problem hiding this comment.
but as far as I can tell PolarisIntegrationTestFixture is the only piece of code that calls purge in tests... no other tests or user-driven actions will get to backend.deleteRealms() on this line... or did I miss something 🤔
There was a problem hiding this comment.
You did ;)
There are tests in a lot of other modules, polaris-nosql-nosql-metastore and many other modules that do not "know" about those types.
Some tests generate a lot of realms, likely one realm per test case. While the amount of data per realm is not much, it is nontheless nice to remove that data immediately (for tests). The maintenance service, which purges data of eligible realms, cannot be run against the in-memory backend (different JVM). This change adds a rather "test only" workaround to purge the realm data in the in-memory backend immediately.
* NoSQL: reduce heap pressure when running tests (apache#3267) Some tests generate a lot of realms, likely one realm per test case. While the amount of data per realm is not much, it is nontheless nice to remove that data immediately (for tests). The maintenance service, which purges data of eligible realms, cannot be run against the in-memory backend (different JVM). This change adds a rather "test only" workaround to purge the realm data in the in-memory backend immediately. * fix(deps): update dependency org.projectnessie.cel:cel-bom to v0.6.0 (apache#3356) * chore: Suppress unchecked case from mock (apache#3322) This is to fix javac `warning: [unchecked] unchecked conversion TriConsumer<String, TableIdentifier, MetricsReport> mockConsumer = mock(TriConsumer.class);` * Add Polaris Console on the tools set (apache#3355) * Move remaining build-time server properties to runtime/defaults (apache#3341) The `runtime/defaults` module is meant as a holder of default Polaris Server runtime properties. This change moves a few remaining properties from the `application.properties` file under `runtime/server` into `runtime/defaults` to avoid any possible confusion regarding the location of effective server properties. Note: The Admin tool has its own `application.properties` file. * Deprecate untyped `RealmConfig.getConfig()` (apache#3323) * `getConfig(String)` has a generic return type, but the call path that gets the value does not perform any type validation. * Deprecate this method in favour of well-typed `getConfig(PolarisConfiguration)` * Migrate the single use case in Polaris code to the well-typed method. * fix(deps): update dependency io.smallrye.common:smallrye-common-annotation to v2.15.0 (apache#3365) * Fix error handler parameters in TaskExecutorImpl (apache#3358) Use correct exception variable inside `CompletableFuture.exceptionallyComposeAsync()` * fix(deps): update dependency org.junit:junit-bom to v5.14.2 (apache#3363) * fix(deps): update dependency com.gradleup.shadow:shadow-gradle-plugin to v9.3.1 (apache#3361) * Propagate previous task exceptions as "suppressed" (apache#3367) * Propagate previous task exceptions as "suppressed" Task retries may fail in different ways in each attempt, however only the last exception used to be exposed to the caller. This change propagates exceptions from all previous tasks execution attempts as "suppressed" exceptions chained to the final tasks failure exception. * Introduce idempotency records schema and Postgres-backed IdempotencyStore (apache#3205) This PR adds the persistence foundation for REST idempotency in Polaris: Defines an idempotency_records table (Postgres) with key, binding, liveness, and finalization fields. Introduces a storage‑agnostic IdempotencyRecord model and IdempotencyStore SPI in polaris-core. Implements PostgresIdempotencyStore in polaris-relational-jdbc * fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.24 (apache#3369) * fix(deps): update immutables to v2.12.1 (apache#3368) * Remove unnecessary version spec in jdbc persistence build file (apache#3373) The testcontainers BOM is already included, so the version spec, which doesn't match the BOM version, is unnecessary. * Fix typo (apache#3376) * Cosmetic: sort lines in libs.versions.toml (apache#3133) * fix(deps): update dependency com.github.dasniko:testcontainers-keycloak to v4.1.0 (apache#3375) * Remove Admin tests from required_status_checks (apache#3370) * fix(deps): update quarkus platform and group to v3.30.6 (apache#3374) * [Bug] Fix a bug that causing error when setting `write.data.path` to be a subdirectory of the table location (apache#3371) Currently, when updating write.data.path of the table to a subdir under the table location, it will fail the location overlap check. For example spark-sql> ALTER TABLE tb1 SET TBLPROPERTIES ( 'write.data.path' = '<tableLocation>/alternative_data' ); org.apache.iceberg.exceptions.ForbiddenException: Forbidden: Unable to create table at location 's3://<table_location>' because it conflicts with existing table or namespace at location 's3://<table_location>` IcebergCatalog.validateNoLocationOverlap(...) constructs a virtual PolarisEntity for overlap checking, but it did not set the entity name. When fetching the siblings of the table, it fails to filter out itself and thus the check mistaken considered that the write.data.path conflict with the table's own base location. (isChildOf) This PR fix the issue by adding name to the virtual PolarisEntity and add a unit and a integration test. * Update release guide to reference the proposed vote e-mail (apache#3377) Now that apache#3150 has been merged, a VOTE e-mail is pre-generated by the third workflow so that there is less room for error. The release guide has been updated to reflect this. There is no pre-generated Incubator vote e-mail as part of the third workflow. This is acceptable in that eventually, after graduation, that would become unnecessary. But also that it could cause confusion as there would be two proposed e-mail bodies in the third workflow. Recommendation is to let the release manager created the Incubator vote e-mail manually. * Last merged commit b324fc3 --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Huaxin Gao <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Honah (Jonas) J. <[email protected]> Co-authored-by: Pierre Laporte <[email protected]>
Some tests generate a lot of realms, likely one realm per test case. While the amount of data per realm is not much, it is nontheless nice to remove that data immediately (for tests).
The maintenance service, which purges data of eligible realms, cannot be run against the in-memory backend (different JVM).
This change adds a rather "test only" workaround to purge the realm data in the in-memory backend immediately.