Re: [PR] Remove dropwizard-jackson dep from core [polaris]
snazy merged PR #470: URL: https://github.com/apache/polaris/pull/470 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #470: URL: https://github.com/apache/polaris/pull/470#issuecomment-2508037339 rebased and resolved conflicts -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #470: URL: https://github.com/apache/polaris/pull/470#issuecomment-2500943492 @collado-mike : What's your take on this alternative approach? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #435: URL: https://github.com/apache/polaris/pull/435#issuecomment-2500895782 Closing in favour of #470 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b closed pull request #435: Remove dropwizard-jackson dep from core URL: https://github.com/apache/polaris/pull/435 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #435: URL: https://github.com/apache/polaris/pull/435#issuecomment-2495267242 Alternative implementation: #470 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #470: URL: https://github.com/apache/polaris/pull/470#issuecomment-2495267429 This is an alternative to #435 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1844262489
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
> 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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
adutra commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1844313254
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
(and in any case, I'd avoid introducing a "polaris-core-dropwizard" module
just for that.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
adutra commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1844312306
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
eric-maynard commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1843197878
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1843126168
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1842802708
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
ok, will do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
adutra commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1842390335
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
I think he meant to enhance the test with some fields populated through
configuration:
```java
@Nested
class EclipseLinkMetastore {
private final DropwizardAppExtension app =
new DropwizardAppExtension<>(
PolarisApplication.class,
CONFIG_PATH,
RANDOM_APP_PORT,
RANDOM_ADMIN_PORT,
ConfigOverride.config("metaStoreManager.type", "eclipse-link"),
ConfigOverride.config("metaStoreManager.persistence-unit",
"test"),
ConfigOverride.config("metaStoreManager.conf-file",
"/path/to/persistence.xml"));
@Test
void testMetastoreType() {
assertThat(app.getConfiguration().getMetaStoreManagerFactory())
.asInstanceOf(type(EclipseLinkPolarisMetaStoreManagerFactory.class))
.extracting("persistenceUnitName", "confFile")
.containsExactly("test", "/path/to/persistence.xml");
}
}
```
(you need to switch the eclipse-link dependency from `testRuntimeOnly` to
`testImplementation`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1842292161
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
The new test in this PR demonstrates config polymorphism between two
different implementations (coming from different modules/jars). @eric-maynard :
could you give a bit more details on what you'd like to be added to tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1842311071
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
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 not 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1842311071
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
collado-mike commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1841638917
##
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/DiscoverableMetaStoreManagerFactory.java:
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.impl.eclipselink;
+
+import io.dropwizard.jackson.Discoverable;
+import io.dropwizard.jackson.DiscoverableSubtypeResolver;
+
+/**
+ * Discoverability aid for Dropwizard. Leaf classes cannot implement {@link
Discoverable} directly,
+ * they have to implement another interface, which implements {@link
Discoverable}.
+ *
+ * @see DiscoverableSubtypeResolver
+ */
+public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Review Comment:
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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
eric-maynard commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1841619831
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
The changes look good to me, but the test doesn't include any configs
specific to that implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #435: URL: https://github.com/apache/polaris/pull/435#issuecomment-2472058528 I rebased and refactored service descriptions (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 ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on PR #435: URL: https://github.com/apache/polaris/pull/435#issuecomment-2471245193 I'm going to add one more commit and rebase presently... hopefully clarifying the use of `Discoverable`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1838190957
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
@eric-maynard : WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833562912
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
@eric-maynard : I've added `PolarisApplicationConfigurationTest` in the
latest commit. Please have a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
eric-maynard commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833443142
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
My understanding is that removing Discoverable from this will break any
implementations that want to supply their own configs (I think that's the
polymorphic configuration mentioned above?).
If you can provide such an implementation in a test and the configs still
get picked up, I think we would be good to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
sfc-gh-emaynard commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833442890
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
My understanding is that removing `Discoverable` from this will break any
implementations that want to supply their own configs (I think that's the
polymorphic configuration mentioned above?).
If you can provide such an implementation in a test and the configs still
get picked up, I think we would be good to merge
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
My understanding is that removing `Discoverable` from this will break any
implementations that want to supply their own configs (I think that's the
polymorphic configuration mentioned above?).
If you can provide such an implementation in a test and the configs still
get picked up, I think we would be good to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833431302
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
@eric-maynard : what other test can validate correct operation without
implementing `Discoverable`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833399333
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
the other relevant service declaration is
https://github.com/apache/polaris/blob/8cb6b44bf57dc597dab612d109d3eb534aef5715/polaris-service/src/main/resources/META-INF/services/org.apache.polaris.core.persistence.MetaStoreManagerFactory#L21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833397309
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
FWIW, listing the factory class in
`META-INF/services/io.dropwizard.jackson.Discoverable` _is_ important, but
implementing `Discoverable` is not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
dimas-b commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833378687
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
well, it is loaded only in one place on startup (as far as I can tell) and I
did verify that call path manually. Jackson was able to deserialize config and
the store impl. correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
eric-maynard commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833363872
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
Indeed I think we cannot safely remove `Discoverable` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Remove dropwizard-jackson dep from core [polaris]
adutra commented on code in PR #435:
URL: https://github.com/apache/polaris/pull/435#discussion_r1833342877
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##
@@ -33,7 +32,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
Review Comment:
Interesting, isn't this going to disturb Dropwizard's so-called
["polymorphic
configuration"](https://www.dropwizard.io/en/release-3.0.x/manual/configuration.html#polymorphic-configuration)?
It seems we rely heavily on that feature though:
https://github.com/apache/polaris/blob/7b2cca4add23e7be369971774a55ce3dae7cca14/polaris-service/src/main/resources/META-INF/services/io.dropwizard.jackson.Discoverable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
