nit: improve UX when running Polaris with Gradle #977
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adutra
requested review from
MonkeyCanCode,
RussellSpitzer,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
flyrain
reviewed
Feb 10, 2025
quarkus/server/build.gradle.kts
Outdated
Contributor
There was a problem hiding this comment.
Log file would be helpful in case of debugging.
Contributor
Author
There was a problem hiding this comment.
Thought about that too, but do you really think people would need to debug these runs after the fact? I think the console logs are enough.
Contributor
Author
There was a problem hiding this comment.
The other problem with file logging is that they tend to pile up and you could end up with tons of .tgz rotated files in the ./logs directory.
Contributor
There was a problem hiding this comment.
It would be helpful for debugging in the following cases:
- Local integration tests.
- Multi-steps operations are involved.
I would keep it if it doesn't bother us too much. Does it?
Contributor
There was a problem hiding this comment.
The other problem with file logging is that they tend to pile up and you could end up with tons of .tgz rotated files in the ./logs directory.
This is a reasonable complaint, can we make it use /tmp somehow?
Contributor
There was a problem hiding this comment.
I will just use rm -rf ./logs to clean it up. It doesn't bother me too much.
Contributor
There was a problem hiding this comment.
Just my $.02, but I have never ever used the log files
Contributor
Author
There was a problem hiding this comment.
Thanks all for the feedback, I am starting to wonder whether the cure is worse than the disease, IOW, maybe the best thing to do here is to not do anything.
Each developer can handle this in their own ways, e.g. by removing the log files periodically as suggested by @flyrain, or by just appending -Dquarkus.log.file.enable=false manually to ./gradlew run (which is probably what I would do).
So, if you all agree, let's just close this PR without merging it.
Contributor
There was a problem hiding this comment.
Thanks @adutra for keeping working on it. Can we still keep the color change?
Contributor
Author
There was a problem hiding this comment.
Oh sure, that's also an option. Let me update the PR then.
Two minor improvements: - Enable colors in the logs - Disable writing log files (console logging is enough)
adutra
force-pushed
the
quarkus-run-enhancements
branch
from
23361ad to
a312e0d
Compare
adutra
force-pushed
the
quarkus-run-enhancements
branch
from
a312e0d to
d97a4b4
Compare
flyrain
approved these changes
Feb 22, 2025
eric-maynard
added a commit
to eric-maynard/polaris
that referenced
this pull request
Mar 13, 2025
* Publish proposals and roadmap on the website (apache#1043) * Allow Polaris CLI to set property values that contain '=' (apache#1003) Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='. This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string. * main: Update postgres Docker tag to v17.4 (apache#1049) * main: Update dependency org.junit:junit-bom to v5.12.0 (apache#1045) * Revert "Remove CallContext and its ThreadLocal usage" (apache#1000) * revert b84f462 * resolve conflicts * autolint * autolint * ...not possible to automatically add a synthetic no-args constructor to an unproxyable bean class. * fix * fixed non-quarkus build issues * fixed non-quarkus build issues * autolint * more di issues * add more producers * add another producer * autolint * change callcontext usage in FileIOFactoryTest * stablize tests * autolint * fix a test * refactor TableCleanupTaskHandlerTest * another fix, still npe * autolint * only task cleanup is problematic * autolint * Stabilize more tests * autolint * Avoid propagating old callcontexts across TestServices instances * testservices update * autolint * Revert "autolint" This reverts commit 66a7f22. * Revert "testservices update" This reverts commit 897a5a1. * changes to taskexecutorimpl * autolint --------- Co-authored-by: Dennis Huo <[email protected]> * Enable console color when running Polaris with Gradle (apache#977) * CLI: add subcommand access for principals (apache#1019) * Access subcommand access for principals * Access subcommand access for principals * Fix getting start spark (apache#1054) * Use correct link format for adoc and update menu title for proposals (apache#1051) * Add a debug log with the full exception (apache#1023) * clean up * spotless * clean up * refactor message * no CVEM * change log levels * revert polaris-java dep change * add testImplementation * uppercase * Remove all tightly coupled EntityCache dependencies in the main persistence stack (apache#1055) Remove the EntityCacheEntry wrapper since the timestamp fields were never used anyways; instead the underlying Caffeine cache transparently handles access times, and the types of entries we cache are simply the existing ResolvedPolarisEntity. Remove interactions of business logic with explicit "cache entries", instead operating on ResolvedPolarisEntity. Fix the equals()/hashCode() behavior of PolarisEntity to be compatible with PolarisBaseEntity as intended. Improve code comments to explain the (current) relationship between PolarisEntity and PolarisBaseEntity, and clarify the behaviors in Resolver.java. Fully remove the PolarisRemoteCache interface and its methods. Add different methods that aren't cache-specific instead. * Remove PolarisMetaStoreSession from FileIOFactory/FileIOUtil in favor of CallContext (apache#1057) This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory. This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest. * Cleanup Iceberg Manifest list files on Purge (apache#1039) * Fix small quickstart typo (apache#1061) * Enable rat check on md files and fix the existing ones (apache#1050) * main: Update dependency boto3 to v1.37.0 (apache#1052) * main: Update dependency software.amazon.awssdk:bom to v2.30.27 (apache#1053) * Integration tests: improve support for temporary folders (apache#987) 3 integration tests currently require a temporary folder for various purposes. The folders are generally hard-coded to some local filesystem location, which makes those tests impossible to execute against a remote Polaris instance. This PR aims at fixing this situation by leveraging a new optional environment variable that points to a location that is accessible by both the server and the client running the tests. This would typically be an S3 bucket, for example, but could also be a shared volume mounted on both machines at the same mount point. If this environment variable is not present, then local execution is assumed and a temporary directory managed by JUnit is used instead. * build-logic: exclude generated code from errorprone (apache#1035) * build-logic: let javadoc depend on jandex (apache#1034) * Add Prashant in the collaborators list (apache#1064) * main: Update dependency org.slf4j:slf4j-api to v2.0.17 (apache#1065) --------- Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: MonkeyCanCode <[email protected]> Co-authored-by: Andrew Guterman <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Robert Stupp <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two minor improvements: