Remove dropwizard-jackson dep from core

[dimas-b]

This is a simplification / cleanup. The dependency does not appear to be required in polaris-core

How Has This Been Tested?

Manual bootstrap execution with EclipseLink and PostgreSQL.

[dimas-b]

I'm going to add one more commit and rebase presently... hopefully clarifying the use of Discoverable.

[dimas-b]

I rebased and refactored service descriptors (resources) to be coherent both with java type hierarchies and comply with the Dropwizard way of discovering Jackson sub-types.

As a side benefit, the last commit fixes this message that happens when running without EclipseLink:

INFO  [2024-11-12 19:41:53,334 - 12224 ] [main] [] i.d.j.DiscoverableSubtypeResolver: Unable to load org.apache.polaris.extension.persistence.impl.eclipselink.EclipseLinkPolarisMetaStoreManagerFactory 

[adutra]

LGTM – I think you achieved a nice balance between removing DW from core but keeping Discoverable around in relevant modules. 👍

[collado-mike]

This doesn't make a lot of sense to me. When the subtype resolver is asked for implementations, it'll be for a common interface - i.e., MetaStoreManagerFactory in this case. Why would the subtype resolver ever ask for the eclipse-link specific version of the DiscoverableMetaStoreManagerFactory?

[dimas-b]

Type resolution during YAML parsing is handled by Jackson, and it properly recognizes all types in the java hierarchy (even those not implementing Discoverable).

However, Jackson (in this setup) relies on DiscoverableSubtypeResolver to discover implementation classes. That latter part has peculiar logic, where DiscoverableSubtypeResolver performs a two stage lookup: 1) classes listed in META-INF/services/io.dropwizard.jackson.Discoverable, 2) classes listed in the service manifests for the classes from step 1. I do not really know why it does not directly inject classes from step 1 into Jackson.

I agree, it looks odd. However, having one Discoverable interface is not possible as this PR specifically proposes to remove DW dependencies from polaris-core, where the base interface exists and there is no other common root between the "in memory" metastore implementation and the EclipseLink implementation. We could add a rather small common module for that, of course, but in my mind having two separate Discoverable interfaces was less intrusive. WDYT?

[eric-maynard]

Leaf classes cannot implement {@link Discoverable} directly, they have to implement another interface, which implements {@link Discoverable}.

Isn't this the exact setup we have right now, where the leaf classes implement MetastoreManagerFactory?

Should we just move MetastoreManagerFactory out of core?

[dimas-b]

Should we just move MetastoreManagerFactory out of core?

That's an option. This is what I meant by "add a rather small common module" above, because the EclipseLink and In-Memory implementations have to share it.

Would do other people think? @collado-mike @adutra ?

[adutra]

So, if I get this correctly, we would all love to have MetastoreManagerFactory directly in polaris-service like many other similar beans, but we can't, because of EclipseLink.

If that is correct, I'd prefer to keep your current solution for now, then revisit this later, when/if EclipseLink is replaced with some more robust persistence solution.

[adutra]

(and in any case, I'd avoid introducing a "polaris-core-dropwizard" module just for that.)

[dimas-b]

Alternative implementation: #470

[dimas-b]

Closing in favour of #470