dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666135805
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java:
##########
@@ -39,6 +40,11 @@ public ProductionReadinessCheck checkRelationalJdbc(
return ProductionReadinessCheck.OK;
}
+ // Skip check if datasource is not available (e.g., in CLI mode before
configuration)
+ if (dataSource.isUnsatisfied() || dataSource.isAmbiguous()) {
+ return ProductionReadinessCheck.OK;
Review Comment:
This is sub-optimal, IMHO. Could we not make readiness checks work normally
for CLI too? It is fine to run different sets of checks for Admin CLI and
Server runtime, but special cases like this in the main code look awkward to me.
##########
runtime/server/build.gradle.kts:
##########
@@ -40,16 +40,36 @@ val distributionElements by
dependencies {
implementation(project(":polaris-runtime-service"))
+ // Dependencies from merged polaris-admin module
+ implementation(project(":polaris-core"))
Review Comment:
Is this necessary? I suppose `runtime/server` included `polaris-core` before
🤔
##########
runtime/server/src/main/java/org/apache/polaris/admintool/config/AdminToolProducers.java:
##########
@@ -33,6 +35,15 @@
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
+/**
+ * CDI producer for admin tool mode. These beans provide minimal
implementations for CLI operations
+ * and take precedence over service beans when running admin commands.
+ *
+ * <p>Note: This class uses @Alternative with @Priority to override
ServiceProducers beans that
+ * would conflict (Clock, PolarisDiagnostics, MetaStoreManagerFactory).
+ */
+@Alternative
+@Priority(100) // Higher priority than default to override ServiceProducers
Review Comment:
I'm not sure how CDI actually works now that `AdminToolProducers` and
`ServiceProducers` are part of the same class path 🤔
@flyrain : do you have any insight? (I did not have time to debug personally
😅 )
##########
.github/workflows/release-3-build-and-publish-artifacts.yml:
##########
@@ -295,7 +295,8 @@ jobs:
run: |
source "${LIBS_DIR}/_exec.sh"
- exec_process ./gradlew :polaris-admin:assemble
:polaris-admin:quarkusAppPartsBuild --rerun \
+ # polaris-admin merged into polaris-server - admin build handled by
server build
+ exec_process ./gradlew :polaris-server:assemble
:polaris-server:quarkusAppPartsBuild --rerun \
Review Comment:
That said, I'd propose to tackle release workflow changes separately (for
ease of review)
##########
runtime/server/src/main/docker/Dockerfile.jvm:
##########
@@ -19,7 +19,7 @@
FROM registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24-2
LABEL org.opencontainers.image.source=https://github.com/apache/polaris \
- org.opencontainers.image.description="Apache Polaris (incubating)" \
+ org.opencontainers.image.description="Apache Polaris (incubating) -
Server and Admin Tool" \
Review Comment:
nit: I think the old description is sufficient
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java:
##########
@@ -48,6 +54,9 @@ public ProductionReadinessCheck checkRelationalJdbc(
"The current persistence (jdbc:h2) is intended for tests
only.",
"quarkus.datasource.jdbc.url"));
}
+ } catch (InactiveBeanException e) {
+ // Datasource is inactive (e.g., in CLI mode before configuration)
+ return ProductionReadinessCheck.OK;
Review Comment:
(related to previous comment)... especially returning OK when something was
not "active" 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -75,6 +75,10 @@ public void warnOnFailedChecks(
@Observes Startup event,
Instance<ProductionReadinessCheck> checks,
ReadinessConfiguration config) {
+ // Skip production readiness checks in CLI mode - they're only relevant
for server deployments
+ if ("cli".equals(System.getProperty("quarkus.profile"))) {
Review Comment:
Let's try to find a more natural way to perform readiness checks under the
CLI env.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -18,8 +18,10 @@
*/
package org.apache.polaris.persistence.relational.jdbc;
+import io.smallrye.config.ConfigMapping;
import java.util.Optional;
+@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
Review Comment:
Is this change required for the admin/server merge? If possible, let's
separate different concerns in different PRs.
##########
.asf.yaml:
##########
@@ -56,7 +56,7 @@ github:
- "Unit Tests"
- "Quarkus Tests"
- "Quarkus Integration Tests"
- - "Quarkus Admin Tests"
+ - "Quarkus Server Tests"
Review Comment:
I believe we simply need to remove `Quarkus Admin Tests` here, not rename
##########
.github/workflows/gradle.yml:
##########
@@ -65,7 +65,7 @@ jobs:
run: |
./gradlew check sourceTarball distTar distZip publishToMavenLocal \
-x :polaris-runtime-service:test \
- -x :polaris-admin:test \
+ -x :polaris-server:test \
Review Comment:
Why would we exclude server tests here? Should we just remove the old admin
exclude?
##########
runtime/server/src/main/java/org/apache/polaris/admintool/PolarisAdminTool.java:
##########
@@ -36,8 +33,8 @@ public class PolarisAdminTool extends BaseMetaStoreCommand {
@Override
public Integer call() {
- PrintWriter out = spec.commandLine().getOut();
- spec.commandLine().usage(out);
+ // When invoked without a subcommand, show usage
Review Comment:
How is it possible to invoke the Admin CLI _without_ a subcommand now? 🤔
##########
runtime/defaults/src/main/resources/application.properties:
##########
@@ -293,3 +293,15 @@ quarkus.index-dependency.protobuf.artifact-id=protobuf-java
# force the locale, just in case the system's using another default locale
quarkus.default-locale=en_US
+# ---- CLI Mode Configuration (activated via -Dquarkus.profile=cli) ----
Review Comment:
+1 to `application-cli.properties`
##########
.github/workflows/release-3-build-and-publish-artifacts.yml:
##########
@@ -295,7 +295,8 @@ jobs:
run: |
source "${LIBS_DIR}/_exec.sh"
- exec_process ./gradlew :polaris-admin:assemble
:polaris-admin:quarkusAppPartsBuild --rerun \
+ # polaris-admin merged into polaris-server - admin build handled by
server build
+ exec_process ./gradlew :polaris-server:assemble
:polaris-server:quarkusAppPartsBuild --rerun \
Review Comment:
If Admin is merged into the server, it should probably not have a separate
docker image? 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
