Merge polaris-service-common into polaris-runtime-service #2233

adutra · 2025-08-01T12:13:20Z

As discussed in the ML here is a PR that merges polaris-service-common into polaris-runtime-service.

99% of the changes are file moves or renamings:

The first commit merges modules
The second commit merges packages
The third commit renames classes

Commits can be reviewed independently.

Only the 3rd one contains changes of interest.

For example, all classes like QuarkusXYZConfiguration were merged into XYZConfiguration if such class exists, rather than renamed.

One particular interface was problematic: FeaturesConfiguration and its subtype ResolvedFeaturesConfiguration. The merger resulted in a cyclic reference when resolving CDI beans. The same issue also happened with BehaviorChangesConfiguration and ResolvedBehaviorChangesConfiguration.

To solve both cyclic reference issues, I created the following hierarchy:

BTW @eric-maynard what is BehaviorChangesConfiguration used for? It seems unused.

Also: I renamed QuarkusProducers in both modules that had such class. They became: ServiceProducers and AdminToolProducers. I also renamed QuarkusSparkIT to SparkIT.

adutra · 2025-08-01T13:19:11Z

The failing test does not fail to me locally: testCreateGcpCredentialsFromStaticToken – I am investigating.

EDIT: not failing anymore 😆 🤷‍♂️

dimas-b

LGTM in general, but I did not check every single file 😅

adutra · 2025-08-02T11:24:52Z

The failing test does not fail to me locally: testCreateGcpCredentialsFromStaticToken – I am investigating.

EDIT: not failing anymore 😆 🤷‍♂️

Failing again :-(

The error is:

Expecting actual: 1754133695923L to be between: [1754133696234L, 1754133697234L]

IOW, the token expires 311 milliseconds before the lowest bound.

I think this test is probably flaky: If a GC pause happens between the token issuance and the call to Instant.now(), we can have more than 500 milliseconds of difference. I will provide a fix.

adutra · 2025-08-02T11:25:21Z

I did not check every single file 😅

I hope you didn't ! 😆

adutra · 2025-08-02T13:40:47Z

FYI: the flaky test should be fixed by #2241.

snazy · 2025-08-04T08:02:03Z

runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java

 * {@link CallContext}.
 */
+@ApplicationScoped
 public class TaskExecutorImpl implements TaskExecutor {


Suggested change

public class TaskExecutorImpl implements TaskExecutor {

class TaskExecutorImpl implements TaskExecutor {

Probably similar for other implementations.

If it works, the constructors can be package-private as well.

snazy · 2025-08-04T08:06:36Z

...me/service/src/main/java/org/apache/polaris/service/config/BehaviorChangesConfiguration.java

+package org.apache.polaris.service.config;
+
+/**
+ * FIXME: this seems unused.


Maybe remove?

Well, I added this FIXME because I'm truly puzzled by this class. @eric-maynard what is this used for?

...service/src/main/java/org/apache/polaris/service/config/RawBehaviorChangesConfiguration.java

runtime/service/src/main/java/org/apache/polaris/service/config/RawFeaturesConfiguration.java

runtime/service/src/main/java/org/apache/polaris/service/config/JacksonConfig.java

snazy · 2025-08-04T08:13:28Z

runtime/service/src/main/java/org/apache/polaris/service/legacy/LegacyManagementEndpoints.java

 * under the License.
 */
-package org.apache.polaris.service.quarkus.legacy;
+package org.apache.polaris.service.legacy;


Unrelated to this PR:
I suspect we can remove this one for the next release.

snazy · 2025-08-04T08:26:39Z

runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java

  private final TaskFileIOSupplier fileIOSupplier;
  private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
  private final PolarisEventListener polarisEventListener;
+  private final Tracer tracer;


I'd prefer to make this a

Suggested change

private final Tracer tracer;

private final Optional<Tracer> tracer;

I'm a bit cautious when using otel code, been bitten hard w/ static initializers in the past, although the particular test doesn't seem to harm.

But how would I inject this, Instance<Tracer>? I'm not sure that's better.

Hm,
Instance.select().stream().findFirst() seems to be the only way...

...e/src/test/java/org/apache/polaris/service/auth/external/PrincipalAuthInfoAugmentorTest.java

snazy · 2025-08-04T08:37:01Z

runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java

  public void testFullExceptionIsLogged(
      ExceptionMapper<Exception> mapper, Exception exception, Level level) {
    Logger logger = (Logger) LoggerFactory.getLogger(mapper.getClass());
+    logger.setLevel(level);


Huh? What's the reason for adding this?

This test is brittle because it assumes that the logger has the tested level enabled, but this is not the case anymore.

Anyways, this test is not very Quarkus-friendly. I will refactor it later.

snazy

LGTM!

adutra · 2025-08-05T13:26:51Z

Hi, since this PR is generating constant rebase conflicts with other ongoing PRs, I'm going to go ahead and merge it. I hope that's ok for everybody. 🙏

* feat: Add `endpointInternal` to `AwsStorageConfigInfo` (apache#2213) * feat: Add `endpointInternal` to `AwsStorageConfigInfo` This API change is backward compatible with older clients and server using old storage configuration. * The `endpointInternal` allows Polaris Servers to use a different host name (or IP address) for accessing S3 storage than clients. This is not a common use case, but may be relevant is more complex environments. * If not set `endpointInternal` defaults to `endpoint`. * The STS endpoint default changes to `endpointInternal`. Contributes to apache#1530 * Fix deprecated Quarkus log properties (apache#2216) see https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.19#other-changes-gear-white_check_mark CI Quarkus tests were logging this repeatedly: ``` The "quarkus.log.file.json" config property is deprecated and should not be used anymore. The "quarkus.log.console.json" config property is deprecated and should not be used anymore. The "quarkus.log.file.json" config property is deprecated and should not be used anymore. The "quarkus.log.console.json" config property is deprecated and should not be used anymore. ``` * chore(deps): update dependency mypy to >=1.17, <=1.17.1 (apache#2218) * fix(deps): update dependency boto3 to v1.40.1 (apache#2240) * fix(deps): update dependency com.azure:azure-sdk-bom to v1.2.37 (apache#2242) * JDBC: Log SQL statements at debug level (apache#2221) * Add ResolutionManifestFactory (apache#2210) after 95358a9 very little functionality was left in `PolarisEntityManager`. by splitting out the more dedicated `ResolutionManifestFactory` we can remove `PolarisEntityManager` and `RealmEntityManagerFactory` completely it seems. * Prepare upgrade to Gradle 9 (apache#2237) * `shadowPub.kt` the change removes a special case that doesn't apply to Polaris * `api/...` build scripts - changes due to nullable type handling (`Property<String>` vs `Property<String?>` - latter is ... weird) * fix(deps): update dependency software.amazon.awssdk:bom to v2.32.14 (apache#2246) * Replace TestPolarisMetaStoreManager with Mockito.spy (apache#2230) this takes less code and is more flexible in the future * Remove obsolete information from README-quarkus.md (apache#2252) * fix(deps): update dependency boto3 to v1.40.2 (apache#2256) * fix(deps): update dependency com.gradleup.shadow:shadow-gradle-plugin to v8.3.9 (apache#2260) * fix(deps): update immutables to v2.11.2 (apache#2257) * Only pass `RealmConfig` to `PolarisStorageIntegration` (apache#2234) All `PolarisStorageIntegration` requite only the `RealmConfig`, not the whole `CallContext`. This makes it easier for the new tasks impleemntations (both proposals). * QuarkusProducers: remove unneeded `BasePersistence` producer (apache#2255) * Use application-scope clock when generating GCP credentials (apache#2241) This change also fixes a flaky test: `StorageConfigurationTest.testCreateGcpCredentialsFromStaticToken` * chore(deps): update plugin jetbrains-changelog to v2.4.0 (apache#2264) * chore(deps): update gradle/actions digest to 017a9ef (apache#2265) * Remove config parameter from `PolarisStorageIntegration#getSubscopedCreds` (apache#2235) Instances of `PolarisStorageIntegration` are created for a particular `PolarisStorageConfigurationInfo`, the same value is then passed into `PSI.getSubscopedCreds()`. This change removes the config parameter, as it's already known when `PolarisStorageIntegration` instances are created. * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.22-1.1753981256 (apache#2266) * Make `*StorageConfigurationInfo` types immutable (apache#2236) This change eventually enables usage of the `*StorageConfigurationInfo` in the `StorageCredentialCacheKey` due to the then memoized hash-code values, to eliminate a couple of JSON re-serializations. * JDBC: SERIALIZABLE/EntityNotFoundException (apache#2219) With serializable isolation level, if either the primary-key or the check-constraint are violated, the lookup of the conflicting entity is not guaranteed to yield a non-null result (transaction start matters). This changes works around this situation w/ serializable isolation. * Merge polaris-service-common into polaris-runtime-service (apache#2233) * JdbcMetaStoreManagerFactory determines schemaVersion once per realm (apache#2217) otherwise every call to `MetaStoreManagerFactory.getOrCreateSession` was running the query. * Last merged commit af69d9f --------- Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Christopher Lambert <xn137@gmx.de> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Alexandre Dutra <adutra@apache.org>

github-project-automation bot added this to Basic Kanban Board Aug 1, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 1, 2025

adutra changed the title ~~Merge service common~~ Merge polaris-service-common into polaris-runtime-service Aug 1, 2025

adutra force-pushed the merge-service-common branch from 1c8e65b to 6f80149 Compare August 1, 2025 12:15

adutra force-pushed the merge-service-common branch 2 times, most recently from 86c44ca to fe0dc19 Compare August 1, 2025 19:50

dimas-b previously approved these changes Aug 1, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Aug 1, 2025

adutra force-pushed the merge-service-common branch from fe0dc19 to c8adf89 Compare August 2, 2025 10:49

adutra force-pushed the merge-service-common branch from c8adf89 to a8529e2 Compare August 3, 2025 09:23

snazy reviewed Aug 4, 2025

View reviewed changes

adutra dismissed dimas-b’s stale review via 01b45e4 August 4, 2025 15:10

adutra force-pushed the merge-service-common branch 2 times, most recently from 990ac90 to 284b7d1 Compare August 4, 2025 15:11

adutra mentioned this pull request Aug 4, 2025

Use application-scoped clock when generating GCP credentials #2241

Merged

adutra force-pushed the merge-service-common branch 2 times, most recently from e7f0d2e to c4cc8aa Compare August 5, 2025 09:26

snazy previously approved these changes Aug 5, 2025

View reviewed changes

adutra dismissed snazy’s stale review via 55e6766 August 5, 2025 09:47

adutra force-pushed the merge-service-common branch from c4cc8aa to 55e6766 Compare August 5, 2025 09:47

snazy previously approved these changes Aug 5, 2025

View reviewed changes

adutra dismissed snazy’s stale review via bbf30e4 August 5, 2025 11:36

adutra force-pushed the merge-service-common branch from 55e6766 to bbf30e4 Compare August 5, 2025 11:36

snazy approved these changes Aug 5, 2025

View reviewed changes

adutra added 2 commits August 5, 2025 13:51

Merge modules and delete polaris-service-common

b336e65

Rename packages, remove ".quarkus"

b42ce29

adutra added 3 commits August 5, 2025 13:51

Remove Quarkus prefix from all class names

e5c7c1b

review

df035fa

review

4edd5dc

adutra force-pushed the merge-service-common branch from bbf30e4 to 4edd5dc Compare August 5, 2025 11:51

adutra merged commit 20febda into apache:main Aug 5, 2025
12 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Aug 5, 2025

adutra deleted the merge-service-common branch August 5, 2025 13:27

	public class TaskExecutorImpl implements TaskExecutor {
	class TaskExecutorImpl implements TaskExecutor {

	private final Tracer tracer;
	private final Optional<Tracer> tracer;

Merge polaris-service-common into polaris-runtime-service #2233

Merge polaris-service-common into polaris-runtime-service #2233

Uh oh!

Conversation

adutra commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

adutra commented Aug 2, 2025

Uh oh!

adutra commented Aug 2, 2025

Uh oh!

adutra commented Aug 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

adutra commented Aug 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adutra commented Aug 1, 2025 •

edited

Loading

adutra commented Aug 1, 2025 •

edited

Loading

adutra Aug 4, 2025 •

edited

Loading