-
Notifications
You must be signed in to change notification settings - Fork 349
Merge Polaris-Admin to Polaris-Server #3340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
542d619 to
70ad92f
Compare
|
@flyrain : CI fails? 🤔 |
|
@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:
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 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.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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than silencing the whole class, I'd suggest modifying the maybeBootstrap method as follows:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you effort, @flyrain !
I played with a local build of this PR and it seems generally usable 👍
However, I believe the PR still needs some attention in the areas of production checks and CDI in general.
Adding some more specific comments below.
...rc/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java
Outdated
Show resolved
Hide resolved
.../org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java
Outdated
Show resolved
Hide resolved
.../org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java
Outdated
Show resolved
Hide resolved
| public Integer call() { | ||
| PrintWriter out = spec.commandLine().getOut(); | ||
| spec.commandLine().usage(out); | ||
| // When invoked without a subcommand, show usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it possible to invoke the Admin CLI without a subcommand now? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
runtime/server/build.gradle.kts
Outdated
| implementation(project(":polaris-runtime-service")) | ||
|
|
||
| // Dependencies from merged polaris-admin module | ||
| implementation(project(":polaris-core")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I suppose runtime/server included polaris-core before 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to compileOnly, otherwise the compilation will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I did not mean to imply that the dep. was wrong... I was just wondering 🙂
If core is require for complication, we should probably keep it as an implementation dep.
| 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"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to find a more natural way to perform readiness checks under the CLI env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm open for suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safer to use the following idiom, as more than one profile can be specified with quarkus.profile:
if (ConfigUtils.isProfileActive("cli")) {There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the new commit. Thanks for the suggestion.
.asf.yaml
Outdated
| - "Quarkus Tests" | ||
| - "Quarkus Integration Tests" | ||
| - "Quarkus Admin Tests" | ||
| - "Quarkus Server Tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we simply need to remove Quarkus Admin Tests here, not rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #3370 to unblock this PR.
runtime/server/src/main/java/org/apache/polaris/admintool/config/AdminToolProducers.java
Outdated
Show resolved
Hide resolved
Awesome! Thanks a lot for the suggestion, @adutra! All tests passed now! |
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my POV this PR is almost ready to merge :) I hope a few new minor comments, below could still be squeezed in.
| quarkus.banner.enabled=false | ||
|
|
||
| # Logging configuration - suppress verbose output for CLI | ||
| quarkus.log.level=WARN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we suppress CONSOLE log instead but keep default levels informative in case a user enabled FILE logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open for that, but the WARN level logging was carried from the application.properties in the removed admin module,
| quarkus.log.level=WARN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, sorry, I missed that. Current code is fine for now 👍
| # Get the directory | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| cd "$SCRIPT_DIR/../admin" | ||
| cd "$SCRIPT_DIR/../server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for follow-up: with a single binary package, we may want to rename / restructure the binary dist for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, there are a few places needed to be changed, including doc. I will address them in followups.
| Instance<ProductionReadinessCheck> checks, | ||
| ReadinessConfiguration config) { | ||
| // Skip production readiness checks in CLI mode - they're only relevant for server deployments | ||
| if (ConfigUtils.isProfileActive("cli")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| MetaStoreManagerFactory factory, | ||
| PersistenceConfiguration config, | ||
| RealmContextConfiguration realmContextConfiguration) { | ||
| if (ConfigUtils.isProfileActive("cli")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: I'd prefer to use explicit config flags (set differently in each profile) vs. checking the name of the profile, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3340 (comment)
| Instance<ProductionReadinessCheck> checks, | ||
| ReadinessConfiguration config) { | ||
| // Skip production readiness checks in CLI mode - they're only relevant for server deployments | ||
| if (ConfigUtils.isProfileActive("cli")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# Conflicts: # runtime/admin/src/main/docker/Dockerfile.jvm

Please check the motivation here, https://lists.apache.org/thread/fs7j81kv2kn0wsh05412yk6pp7w7r5nz
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)