You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2025/12/30 22:08:28 UTC
[PR] Merge Polaris-Admin to Polaris-Server [polaris]
flyrain opened a new pull request, #3340:
URL: https://github.com/apache/polaris/pull/3340
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2673036594
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -75,6 +76,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 (ConfigUtils.isProfileActive("cli")) {
Review Comment:
I believe using dedicated config properties for this is a small change :) but I'm also ok with addressing this in a follow-up PR.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2676874594
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
+1, that's actually a nice idea!
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674260084
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
If a user runs both the server and the CLI with the same Polaris binary of the same location, a shared `application.properties` applies to both use cases. In that setup, having a separate `application-cli.properties` still seems useful to capture CLI specific configuration that should not affect the server behavior. That separation feels reasonable and keeps the shared defaults clean while allowing targeted overrides for the CLI. For example, CLI specific configs like the following will be overwritten by `application.properties`
```
quarkus.banner.enabled=false
quarkus.log.level=WARN
```
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674277714
##########
runtime/server/build.gradle.kts:
##########
@@ -40,6 +40,17 @@ val distributionElements by
dependencies {
implementation(project(":polaris-runtime-service"))
+ // Additional dependencies for CLI mode
+ implementation("io.quarkus:quarkus-picocli")
+ implementation(project(":polaris-version"))
+
+ // These are already provided transitively by polaris-runtime-service
+ // but we need them at compile time for CLI code
+ compileOnly(project(":polaris-core"))
Review Comment:
Fixed
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674432446
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
> profile-specific config works in conjunction with the profile-neutral config
That's true, and I can confirm that's how it work today.
> This might be too much low-level details for end users to think about.
This isn't a user-face doc. This is a comment in the source code. I think it should be fine to be here to provide complete view of how it works.
> Even if all the code is bundled in the same Quarkus application, Server and Admin could still have distinct shell entry points and distinct application.properties files (each in its own directory). That would be my preference for the tar-based distribution.
> Docker images can bundle anything that Polaris developers are ok with. In docker, users will override config via external mechanisms like env. variables. If a config map is used to define application.properties it will be specific to a particular runtime scenario anyway (Server or Admin, but not both).
I understand that these particular use cases may not require profile specific config files today. That said, this does not mean the scenarios I mentioned will never occur. Providing clear and explicit messaging still has value in that context. Even if a user never actively uses `External config/application-cli.properties` as a profile specific external config, having a clear understanding of how it works can be important when diagnosing or debugging unexpected behavior.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2681823617
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
I don't think keeping the admin module around is "over-splitting".
This PR, while interesting in that it achieves a single artifact/jar for both usages (server and CLI), does create an imbalanced layout where the top module includes code for the CLI, but the code for the server lives in a dependency:
```mermaid
flowchart TD
A["`polaris-server<br/>(incl. admin tool)`"] --> B
B[polaris-runtime-service]
```
Whereas @dimas-b proposal still keeps the single entrypoint but each usage has its own module:
```mermaid
flowchart TD
A[polaris-server] --> B
A --> C
B[polaris-runtime-service] --> D
C[polaris-admin] --> E
B --> F
C --> F
D[specific deps]
E[specific deps]
F[common deps]
```
This has a net advantage imho of leaving `polaris-server` as a mere "assembly descriptor" as @dimas-b puts it.
> It makes the dependency graph harder to reason about, and forces consumers and tests to pull in modules that exist only to host a thin slice of logic
I don't buy much this argument: while a proliferation of modules can complicate the dependency graph, modularity offers many upsides: modern build tools efficiently manage even hundreds of dependencies; a DAG enforces clearer mental models about dependencies than monoliths that often create dependency cycles; smaller modules accelerate compilation; and downstream integrators benefit from leaner CDI contexts.
Polaris is currently in an intermediate, non-optimal state: it's neither a monolith, nor a well organized modular application. We should rethink the purpose of those "common" modules, but also consider separating disparate concerns in modules like `polaris-runtime-service`: what does telemetry have to do with authentication? And what does the secrets manager have to do with events processing? I support moving towards a well-thought modular organization, and I think @dimas-b suggestion goes in that direction.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666503070
##########
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:
Removed, I think these changes are not needed. Thanks for flagging it!
##########
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:
Same 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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666720168
##########
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:
Sure, I'm open for suggestions.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674260084
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
If a user runs both the server and the CLI the same Polaris binary of the same location, a shared `application.properties` applies to both use cases. In that setup, having a separate `application-cli.properties` still seems useful to capture CLI specific configuration that should not affect the server behavior. That separation feels reasonable and keeps the shared defaults clean while allowing targeted overrides for the CLI.
For example, CLI specific configs like the following will be overwritten by `application.properties`
```
quarkus.banner.enabled=false
quarkus.log.level=WARN
```
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670303401
##########
runtime/distribution/bin/admin:
##########
@@ -21,12 +21,16 @@ set -euo pipefail
# Get the directory
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-cd "$SCRIPT_DIR/../admin"
+cd "$SCRIPT_DIR/../server"
# Get the Java command
JAVA_CMD="${JAVA_HOME:+${JAVA_HOME%/}/bin/java}"
JAVA_CMD="${JAVA_CMD:-$(command -v java)}"
[ -x "$JAVA_CMD" ] || { echo "Java not found โ set JAVA_HOME or add java to PATH." >&2; exit 1; }
-# Launch Quarkus
-exec "$JAVA_CMD" ${POLARIS_JAVA_OPTS:-} -jar quarkus-run.jar "$@"
+# Show the usage when no argument is provided
+if [ "$#" -eq 0 ]; then
+ set -- -h
Review Comment:
Fixed
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677122196
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
I agree on "test ergonomics" as an advantage in the current diff of this PR, but I do not think "modularity" is better - that's exactly what I meant to highlight in my comment above :)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #3340:
URL: https://github.com/apache/polaris/pull/3340#issuecomment-3715881198
@flyrain the CI failure is an interesting one. After some investigation, it seems related to the colored output that we have by default when running server tests:
https://github.com/apache/polaris/blob/c4fc848bce5990ab44151980916a0c704696b79b/runtime/defaults/src/main/resources/application-test.properties#L28
Quarkus CLI tests seem unable to capture colored output properly.
This can be fixed in two ways: by removing the colored output completely for all tests (but that's frustrating) โ or by implementing `getConfigProfile()` in `RelationalJdbcAdminProfile` and using another profile than `test`:
```java
public class RelationalJdbcAdminProfile extends RelationalJdbcProfile {
@Override
public Map<String, String> getConfigOverrides() {...}
@Override
public List<TestResourceEntry> testResources() {...}
// ADD THIS
@Override public String getConfigProfile() { return "cli"; }
}
```
The above will make the test run under the `cli` profile and thus solves the issue. (We might end up needing a `cli-test` profile one day...)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666501425
##########
.asf.yaml:
##########
@@ -56,7 +56,7 @@ github:
- "Unit Tests"
- "Quarkus Tests"
- "Quarkus Integration Tests"
- - "Quarkus Admin Tests"
+ - "Quarkus Server Tests"
Review Comment:
Removed, however, the CI is still asking for it as a pending task. Maybe we should remove it first in a separate PR?
<img width="866" height="252" alt="Screenshot 2026-01-06 at 2 33 35โฏPM" src="https://github.com/user-attachments/assets/18aba737-ba81-46e9-b616-30534a1fd753" />
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670187367
##########
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:
Oh, I did not mean to imply that the dep. was wrong... I was just wondering :slightly_smiling_face:
If core is require for complication, we should probably keep it as an `implementation` dep.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2669880655
##########
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:
Fixed in the new commit. Thanks for the suggestion.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674140698
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
+1 Let's advise users to customize via the profile-neutral `application.properties` (and/or corresponding `-D` and env. vars).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666505962
##########
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:
Added application-cli.properties to host them. Thanks for the suggestion, @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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666495245
##########
.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:
Removed release related changes
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670288554
##########
runtime/distribution/bin/admin:
##########
@@ -21,12 +21,16 @@ set -euo pipefail
# Get the directory
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-cd "$SCRIPT_DIR/../admin"
+cd "$SCRIPT_DIR/../server"
Review Comment:
Definitely, there are a few places needed to be changed, including doc. I will address them in followups.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2681637072
##########
runtime/service/build.gradle.kts:
##########
@@ -33,8 +33,8 @@ dependencies {
runtimeOnly(project(":polaris-relational-jdbc"))
- implementation(project(":polaris-runtime-defaults"))
implementation(project(":polaris-runtime-common"))
+ implementation(project(":polaris-runtime-defaults"))
Review Comment:
Noop change in this big PR.
##########
LICENSE:
##########
@@ -328,7 +328,6 @@ This product includes code from Project Nessie.
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioAccess.java
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioContainer.java
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioExtension.java
-* runtime/admin/src/main/java/org/apache/polaris/admintool/PolarisAdminTool.java
Review Comment:
This file has been moved, not removed.
##########
runtime/defaults/src/main/resources/application.properties:
##########
@@ -292,4 +292,3 @@ 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
-
Review Comment:
Unrelated change to this already big PR.
##########
runtime/admin/distribution/LICENSE:
##########
Review Comment:
The LICENSE file contains entries that are not migrated.
##########
bom/build.gradle.kts:
##########
@@ -89,8 +89,6 @@ dependencies {
api(project(":polaris-extensions-auth-opa"))
- api(project(":polaris-admin"))
- api(project(":polaris-runtime-common"))
Review Comment:
This should not be removed from the bom
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677114592
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
I see the appeal of this approach, but I worry it may dismiss some of the key benefits of the current refactor, and make the dependency structure less clearer. Please note that, the CLI will need `ServiceProvider` to work now.
It also makes testing a bit awkward. Tests that are specific to the CLI behavior would have to depend on the `default` and `service` module just to pick up `application-cli` and `application` properties , which feels indirect and less explicit. For those reasons, I think the current direction still has advantages in terms of modularity, clarity, and test ergonomics.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674088028
##########
runtime/server/build.gradle.kts:
##########
@@ -40,6 +40,17 @@ val distributionElements by
dependencies {
implementation(project(":polaris-runtime-service"))
+ // Additional dependencies for CLI mode
+ implementation("io.quarkus:quarkus-picocli")
+ implementation(project(":polaris-version"))
+
+ // These are already provided transitively by polaris-runtime-service
+ // but we need them at compile time for CLI code
+ compileOnly(project(":polaris-core"))
Review Comment:
Agree with @dimas-b these should be `implementation` for most of them. I would reorder them as below:
```kotlin
implementation(project(":polaris-core"))
implementation(project(":polaris-version"))
implementation(project(":polaris-api-management-service"))
implementation(project(":polaris-api-iceberg-service"))
implementation(project(":polaris-runtime-service"))
compileOnly("com.fasterxml.jackson.core:jackson-annotations")
implementation("io.quarkus:quarkus-picocli")
```
##########
runtime/distribution/bin/admin:
##########
@@ -21,12 +21,16 @@ set -euo pipefail
# Get the directory
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-cd "$SCRIPT_DIR/../admin"
+cd "$SCRIPT_DIR/../server"
# Get the Java command
JAVA_CMD="${JAVA_HOME:+${JAVA_HOME%/}/bin/java}"
JAVA_CMD="${JAVA_CMD:-$(command -v java)}"
[ -x "$JAVA_CMD" ] || { echo "Java not found โ set JAVA_HOME or add java to PATH." >&2; exit 1; }
-# Launch Quarkus
-exec "$JAVA_CMD" ${POLARIS_JAVA_OPTS:-} -jar quarkus-run.jar "$@"
+# Show the usage when no argument is provided
+if [ "$#" -eq 0 ]; then
Review Comment:
Nice ๐
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
As I mentioned on the ML, I'm unsure whether we should mention the possibility of adding this profile-aware external config file, as it has very little added value.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674396182
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
AFAIK, profile-specific config works in conjunction with the profile-neutral config. In other words, `application.properties` is _always_ loaded. This might be too much low-level details for end users to think about.
Even if all the code is bundled in the same Quarkus application, Server and Admin could still have distinct shell entry points and distinct `application.properties` files (each in its own directory). That would be my preference for the tar-based distribution.
Docker images can bundle anything that Polaris developers are ok with. In docker, users will override config via external mechanisms like env. variables. If a config map is used to define `application.properties` it will be specific to a particular runtime scenario anyway (Server or Admin, but not both).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2665805981
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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;
+
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+import jakarta.inject.Inject;
+import org.apache.polaris.admintool.PolarisAdminTool;
+import picocli.CommandLine;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
Review Comment:
I think this could be simplified a bit:
```java
@QuarkusMain
public class PolarisApplication implements QuarkusApplication {
/** Main method that detects CLI mode and activates the appropriate Quarkus profile. */
public static void main(String... args) {
if (args.length > 0) {
System.setProperty("quarkus.profile", "cli");
Quarkus.run(PicocliRunner.class, args);
} else {
Quarkus.run(PolarisApplication.class, args);
}
}
@Override
public int run(String... args) {
Quarkus.waitForExit();
return 0;
}
}
```
I confirmed the above works, if you put back the `@TopCommand` annotation in `PolarisAdminTool`.
##########
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:
Rather than adding properties here, wdyt about introducing a profile-aware file `application-cli.properties`? I think this would more clearly demarcate the configuration for CLI from the one for server (which is also the default).
https://quarkus.io/guides/config-reference#profile-aware-files
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670190463
##########
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:
sweet :+1:
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670291390
##########
runtime/defaults/src/main/resources/application-cli.properties:
##########
@@ -0,0 +1,42 @@
+#
+# 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.
+#
+
+# ---- CLI Profile Configuration ----
+# This file is automatically loaded when running with -Dquarkus.profile=cli
+# It contains configuration specific to the Polaris Admin CLI commands
+#
+# WARNING: Only RUNTIME configuration properties are allowed in this profile file!
+# Build-time properties (those marked as "fixed at build-time" in Quarkus docs) MUST be
+# defined in the base application.properties file. Profile-specific files like this one
+# can only override runtime configuration values.
+#
+# Setting build-time properties here will have NO EFFECT since the profile is activated
+# after the application has already been built. Build-time properties are baked into the
+# application during the build phase and cannot be changed at runtime.
+#
+# See: https://quarkus.io/guides/config-reference#build-time-configuration
+
+# Application identity
+quarkus.application.name=Apache Polaris Admin Tool (incubating)
+quarkus.banner.enabled=false
+
+# Logging configuration - suppress verbose output for CLI
+quarkus.log.level=WARN
Review Comment:
ah, sorry, I missed that. Current code is fine for now :+1:
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666656662
##########
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:
Good Catch, Thanks @dimas-b ! Removed it. The `ServiceProducers` has covered all of them. No need to have this one.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666653788
##########
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:
Removed
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3340:
URL: https://github.com/apache/polaris/pull/3340#issuecomment-3716901063
> ```java
> public class RelationalJdbcAdminProfile extends RelationalJdbcProfile {
> @Override
> public Map<String, String> getConfigOverrides() {...}
>
> @Override
> public List<TestResourceEntry> testResources() {...}
>
> // ADD THIS
> @Override public String getConfigProfile() { return "cli"; }
> }
> ```
>
> The above will make the test run under the `cli` profile and thus solves the issue. (We might end up needing a `cli-test` profile one day...)
Awesome! Thanks a lot for the suggestion, @adutra! All tests passed now!
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666496226
##########
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:
Removed this change.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2759812747
##########
LICENSE:
##########
@@ -328,7 +328,6 @@ This product includes code from Project Nessie.
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioAccess.java
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioContainer.java
* tools/minio-testcontainer/src/main/java/org/apache/polaris/test/minio/MinioExtension.java
-* runtime/admin/src/main/java/org/apache/polaris/admintool/PolarisAdminTool.java
Review Comment:
Good point. We should just update the path (and possibly move this line if sort order matters).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2676849061
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
Sorry for the last minute comment here, but I wonder if it might make sense to keep the `runtime/admin` module as a home for Admin code, but not build a Quarkus app from it. The `runtime/server` module will then import `runtime/admin` and we end up with one Quarkus app both for the Server and Admin Tool... WDYT?
My rationale for keeping the `runtime/admin` module is just better code isolation at compile time. The `runtime/server` module is mostly an assembly descriptor, ATM it does not have any feature code. It might be preferable to keep it this way to allow simpler reuse of feature code from their dedicated modules.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677122196
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
I agree on "test ergonomics" as an advantage in the current diff of this PR, but I do not think "modularity" is better - that's exactly what I meant to highlight in my comment above :)
IMHO, better modularity is preferable to "test ergonomics".
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666775683
##########
.asf.yaml:
##########
@@ -56,7 +56,7 @@ github:
- "Unit Tests"
- "Quarkus Tests"
- "Quarkus Integration Tests"
- - "Quarkus Admin Tests"
+ - "Quarkus Server Tests"
Review Comment:
Filed https://github.com/apache/polaris/pull/3370 to unblock this PR.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3340:
URL: https://github.com/apache/polaris/pull/3340#issuecomment-3714851093
@flyrain : CI fails? :thinking:
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670309352
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -75,6 +76,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 (ConfigUtils.isProfileActive("cli")) {
Review Comment:
That's probably a good idea, esp. if we want to group a set of methods under the same configure key, instead of spreed `if (ConfigUtils.isProfileActive("cli")` in multiple places. We could improve in followups.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670309940
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -274,6 +275,9 @@ public void maybeBootstrap(
MetaStoreManagerFactory factory,
PersistenceConfiguration config,
RealmContextConfiguration realmContextConfiguration) {
+ if (ConfigUtils.isProfileActive("cli")) {
Review Comment:
See https://github.com/apache/polaris/pull/3340#discussion_r2670309352
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3340: Merge Polaris-Admin to Polaris-Server
URL: https://github.com/apache/polaris/pull/3340
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2665945682
##########
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) ----
+# CLI-specific settings for admin tool commands
+%cli.quarkus.application.name=Apache Polaris Admin Tool (incubating)
+%cli.quarkus.banner.enabled=false
+%cli.quarkus.log.level=WARN
+%cli.quarkus.log.category."io.quarkus".level=ERROR
+%cli.quarkus.log.category."io.quarkus.agroal.deployment".level=ERROR
Review Comment:
nit: unnecessary.
```suggestion
```
##########
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) ----
+# CLI-specific settings for admin tool commands
+%cli.quarkus.application.name=Apache Polaris Admin Tool (incubating)
+%cli.quarkus.banner.enabled=false
+%cli.quarkus.log.level=WARN
+%cli.quarkus.log.category."io.quarkus".level=ERROR
+%cli.quarkus.log.category."io.quarkus.agroal.deployment".level=ERROR
+%cli.quarkus.log.category."org.hibernate.validator".level=ERROR
+%cli.quarkus.log.category."org.apache.polaris.service.config.ServiceProducers".level=ERROR
Review Comment:
Rather than silencing the whole class, I'd suggest modifying the `maybeBootstrap` method as follows:
```java
public void maybeBootstrap(
@Observes Startup event,
MetaStoreManagerFactory factory,
PersistenceConfiguration config,
RealmContextConfiguration realmContextConfiguration) {
if (ConfigUtils.isProfileActive("cli")) {
return; // never bootstrap realms in CLI profile
}
...
```
I'd also suggest moving this method to a separate class, as it is becoming quite big and complex.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666369665
##########
.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:
Sure, let's separate the release workflow to followup PRs
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666373630
##########
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:
It is not 100% necessary. We could finish this PR first, then rebase #3281 to tackle this issue.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2759707401
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
@flyrain : WDYT about this proposal?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666176004
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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;
+
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+import jakarta.inject.Inject;
+import org.apache.polaris.admintool.PolarisAdminTool;
+import picocli.CommandLine;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
Review Comment:
+1 to @adutra 's suggestion
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666706183
##########
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:
Both work per my test. I have removed the comment to avoid confusion.
```
yufei@Yufeis-MacBook-Pro-2 polaris % java -jar runtime/server/build/quarkus-app/quarkus-run.jar -h
Usage: polaris-admin-tool.jar [-hV] [COMMAND]
Polaris administration & maintenance tool
-h, --help Show this help message and exit.
-V, --version Print version information and exit.
Commands:
help Display help information about the specified command.
bootstrap Bootstraps realms and root principal credentials.
purge Purge realms and all associated entities.
yufei@Yufeis-MacBook-Pro-2 polaris % java -jar runtime/server/build/quarkus-app/quarkus-run.jar purge -h
Usage: polaris-admin-tool.jar purge [-hV] -r=<realm> [-r=<realm>]...
Purge realms and all associated entities.
-h, --help Show this help message and exit.
-r, --realm=<realm> The name of a realm to purge.
-V, --version Print version information and exit.
```
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670194838
##########
.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:
make sense. I was confused between `runtime/server` and `runtime/service` ๐คฆ
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3340:
URL: https://github.com/apache/polaris/pull/3340#issuecomment-4009091368
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666493520
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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;
+
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+import jakarta.inject.Inject;
+import org.apache.polaris.admintool.PolarisAdminTool;
+import picocli.CommandLine;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
Review Comment:
Good idea, adapted in the new commit.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666717494
##########
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) ----
+# CLI-specific settings for admin tool commands
+%cli.quarkus.application.name=Apache Polaris Admin Tool (incubating)
+%cli.quarkus.banner.enabled=false
+%cli.quarkus.log.level=WARN
+%cli.quarkus.log.category."io.quarkus".level=ERROR
+%cli.quarkus.log.category."io.quarkus.agroal.deployment".level=ERROR
Review Comment:
Removed
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2667686499
##########
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:
It's safer to use the following idiom, as more than one profile can be specified with `quarkus.profile`:
```java
if (ConfigUtils.isProfileActive("cli")) {
```
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670211438
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -75,6 +76,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 (ConfigUtils.isProfileActive("cli")) {
Review Comment:
This is probably ok in the interest of progress, but in general I hope we could leverage profile-specific config like `polaris.production.readiness.checks.enabled=true` (or `false` for CLI). WDYT?
##########
runtime/distribution/bin/admin:
##########
@@ -21,12 +21,16 @@ set -euo pipefail
# Get the directory
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-cd "$SCRIPT_DIR/../admin"
+cd "$SCRIPT_DIR/../server"
# Get the Java command
JAVA_CMD="${JAVA_HOME:+${JAVA_HOME%/}/bin/java}"
JAVA_CMD="${JAVA_CMD:-$(command -v java)}"
[ -x "$JAVA_CMD" ] || { echo "Java not found โ set JAVA_HOME or add java to PATH." >&2; exit 1; }
-# Launch Quarkus
-exec "$JAVA_CMD" ${POLARIS_JAVA_OPTS:-} -jar quarkus-run.jar "$@"
+# Show the usage when no argument is provided
+if [ "$#" -eq 0 ]; then
+ set -- -h
Review Comment:
nit: `--help`?
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -274,6 +275,9 @@ public void maybeBootstrap(
MetaStoreManagerFactory factory,
PersistenceConfiguration config,
RealmContextConfiguration realmContextConfiguration) {
+ if (ConfigUtils.isProfileActive("cli")) {
Review Comment:
Same here: I'd prefer to use explicit config flags (set differently in each profile) vs. checking the name of the profile, please.
##########
runtime/distribution/bin/admin:
##########
@@ -21,12 +21,16 @@ set -euo pipefail
# Get the directory
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-cd "$SCRIPT_DIR/../admin"
+cd "$SCRIPT_DIR/../server"
Review Comment:
for follow-up: with a single binary package, we may want to rename / restructure the binary dist for clarity.
##########
runtime/defaults/src/main/resources/application-cli.properties:
##########
@@ -0,0 +1,42 @@
+#
+# 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.
+#
+
+# ---- CLI Profile Configuration ----
+# This file is automatically loaded when running with -Dquarkus.profile=cli
+# It contains configuration specific to the Polaris Admin CLI commands
+#
+# WARNING: Only RUNTIME configuration properties are allowed in this profile file!
+# Build-time properties (those marked as "fixed at build-time" in Quarkus docs) MUST be
+# defined in the base application.properties file. Profile-specific files like this one
+# can only override runtime configuration values.
+#
+# Setting build-time properties here will have NO EFFECT since the profile is activated
+# after the application has already been built. Build-time properties are baked into the
+# application during the build phase and cannot be changed at runtime.
+#
+# See: https://quarkus.io/guides/config-reference#build-time-configuration
+
+# Application identity
+quarkus.application.name=Apache Polaris Admin Tool (incubating)
+quarkus.banner.enabled=false
+
+# Logging configuration - suppress verbose output for CLI
+quarkus.log.level=WARN
Review Comment:
Should we suppress CONSOLE log instead but keep default levels informative in case a user enabled FILE logging?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2670286466
##########
runtime/defaults/src/main/resources/application-cli.properties:
##########
@@ -0,0 +1,42 @@
+#
+# 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.
+#
+
+# ---- CLI Profile Configuration ----
+# This file is automatically loaded when running with -Dquarkus.profile=cli
+# It contains configuration specific to the Polaris Admin CLI commands
+#
+# WARNING: Only RUNTIME configuration properties are allowed in this profile file!
+# Build-time properties (those marked as "fixed at build-time" in Quarkus docs) MUST be
+# defined in the base application.properties file. Profile-specific files like this one
+# can only override runtime configuration values.
+#
+# Setting build-time properties here will have NO EFFECT since the profile is activated
+# after the application has already been built. Build-time properties are baked into the
+# application during the build phase and cannot be changed at runtime.
+#
+# See: https://quarkus.io/guides/config-reference#build-time-configuration
+
+# Application identity
+quarkus.application.name=Apache Polaris Admin Tool (incubating)
+quarkus.banner.enabled=false
+
+# Logging configuration - suppress verbose output for CLI
+quarkus.log.level=WARN
Review Comment:
I'm open for that, but the `WARN` level logging was carried from the application.properties in the removed `admin` module, https://github.com/apache/polaris/blob/476301b9f8bc20f07ccb1ac9bbb83f88843920d9/runtime/admin/src/main/resources/application.properties#L67. Should we change it now?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677128371
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
We will need a few common modules to enable a separate admin module. I believe this is one of areas we used to struggle with.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674260084
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
If a user runs both the server and the CLI with the same Polaris binary of the same location, a shared `application.properties` applies to both use cases. In that setup, having a separate `application-cli.properties` still seems useful to capture CLI specific configuration that should not affect the server behavior. That separation feels reasonable and keeps the shared defaults clean while allowing targeted overrides for the CLI.
For example, CLI specific configs like the following will be overwritten by `application.properties`
```
quarkus.banner.enabled=false
quarkus.log.level=WARN
```
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666656662
##########
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:
Removed it. The `ServiceProducers` has covered all of them. No need to have this one.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666660574
##########
.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:
All tests now in `polaris-server` come from the removed module `polaris-admin`. This is to keep the behavior unchanged.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666719256
##########
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) ----
+# CLI-specific settings for admin tool commands
+%cli.quarkus.application.name=Apache Polaris Admin Tool (incubating)
+%cli.quarkus.banner.enabled=false
+%cli.quarkus.log.level=WARN
+%cli.quarkus.log.category."io.quarkus".level=ERROR
+%cli.quarkus.log.category."io.quarkus.agroal.deployment".level=ERROR
+%cli.quarkus.log.category."org.hibernate.validator".level=ERROR
+%cli.quarkus.log.category."org.apache.polaris.service.config.ServiceProducers".level=ERROR
Review Comment:
SGTM. Adopted the change.
For moving it to a separate class, I'd suggest to tackle it in a followup PR given the current PR is big already.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2666653196
##########
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:
Change it to `compileOnly`, otherwise the compilation will fail.
##########
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:
Changed it to `compileOnly`, otherwise the compilation will fail.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2674396182
##########
runtime/server/src/main/java/org/apache/polaris/PolarisApplication.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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;
+
+import io.quarkus.picocli.runtime.PicocliRunner;
+import io.quarkus.runtime.Quarkus;
+import io.quarkus.runtime.QuarkusApplication;
+import io.quarkus.runtime.annotations.QuarkusMain;
+
+/**
+ * Main entry point for Polaris application. Supports both HTTP server mode and CLI admin mode: - No
+ * arguments: Start HTTP server - With arguments: Run CLI commands (bootstrap, purge, etc.)
+ */
+@QuarkusMain
+public class PolarisApplication implements QuarkusApplication {
+ /**
+ * Main method that detects CLI mode and activates the appropriate Quarkus profile.
+ *
+ * <p>Profile Activation: When CLI arguments are present, the "cli" profile is activated using
+ * System.setProperty before Quarkus initialization. This must be done via system property (not
+ * ConfigUtils.getProfiles().add()) because Quarkus reads the active profile during early
+ * bootstrap, before ConfigUtils can affect the configuration.
+ *
+ * <p>Configuration Priority Order: Quarkus loads configuration from multiple sources (highest to
+ * lowest priority):
+ *
+ * <ol>
+ * <li>(400) System properties: -D flags at JVM startup
+ * <li>(300) Environment variables
+ * <li>(260) External config/application-cli.properties: Profile-specific external config
Review Comment:
AFAIK, profile-specific config works in conjunction with the provide-neutral config. In other words, `application.properties` is _always_ loaded. This might be too much low-level details for end users to think about.
Even if all the code is bundled in the same Quarkus application, Server and Admin could still have distinct shell entry points and distinct `application.properties` files (each in its own directory). That would be my preference for the tar-based distribution.
Docker images can bundle anything that Polaris developers are ok with. In docker, users will override config via external mechanisms like env. variables. If a config map is used to define `application.properties` it will be specific to a particular runtime scenario anyway (Server or Admin, but not both).
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2676849061
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
Sorry for the last minute comment here, but I wonder if it might make sense to keep the `runtime/admin` module as a home for Admin code, but not build a Quarkus app from it. The `runtime/server` module will then import `runtime/admin` and we end up with one Quarkus app both for the Server and Admin Tool... WDYT?
My rationale for keeping the `runtime/admin` module is just better code isolation at compile time. The `runtime/server` module is mostly an assembly descriptor, ATM it does not have any feature code. It might be preferable to keep it this way to allow simpler reused on feature code from their dedicated modules.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677114592
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
I see the appeal of this approach, but I worry it may dismiss some of the key benefits of the current refactor(we will need a few common modules to enable a separate admin module), and make the dependency structure less clearer. Please note that, the CLI will need `ServiceProvider` to work now.
It also makes testing a bit awkward. Tests that are specific to the CLI behavior would have to depend on the `default` and `service` module just to pick up `application-cli` and `application` properties , which feels indirect and less explicit. For those reasons, I think the current direction still has advantages in terms of modularity, clarity, and test ergonomics.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677180274
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
In terms of modularity, splitting functionalities across multiple modules does not automatically improve modularity. In many cases it does the opposite. It makes the dependency graph harder to reason about, and forces consumers and tests to pull in modules that exist only to host a thin slice of logic. I think good modularity is about cohesion and clear ownership. A module should group functionality that changes together and represents a meaningful boundary. And I'm not a fan of how we layout OPA modules.
In practice, over splitting often trades real modularity for superficial structure, while introducing costs in test ergonomics, dependency clarity, and mental overhead. Keeping related behavior together, even if it is small, is often the more modular choice. I have mentioned that we have to keep introducing new modules like `runtime/common`, `runtime/test-common`, `runtime/distribution`, they are the side effect of over splitting, which can be avoided in the beginning.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merge Polaris-Admin to Polaris-Server [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3340:
URL: https://github.com/apache/polaris/pull/3340#discussion_r2677126633
##########
runtime/server/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
Review Comment:
Testing the admin tool could follow the same approach as testing OPA (which is not monolithic with the server, but a pure puglin)... 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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org