Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use Airlift logger #260

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Use Airlift logger #260

merged 1 commit into from
Mar 7, 2024

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 17, 2024 •
edited

Fixes #147

@cla-bot cla-bot bot added the cla-signed label Feb 17, 2024
@ebyhr ebyhr marked this pull request as draft February 17, 2024 04:06
@mosabua
Copy link
Member

mosabua commented Feb 21, 2024

I like this approach. Not a review .. just an initial thought:

We should we add docs on how to configure logging and add minimal config to jar with dependencies and docker container.

@ebyhr ebyhr force-pushed the ebi/airlift-log branch 5 times, most recently from a013c45 to 1d00947 Compare February 29, 2024 12:56
@ebyhr ebyhr marked this pull request as ready for review February 29, 2024 13:05
@ebyhr ebyhr requested a review from wendigo February 29, 2024 13:06
willmostly
willmostly previously requested changes Mar 6, 2024
View reviewed changes
docs/installation.md Outdated
@@ -105,6 +105,14 @@ configuration YAML file.

Find more information in the [routing rules documentation](routing-rules.md).

### Configure logging

Path to `log.properties` needs to be set via `log.levels-file` JVM options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added to release notes as a breaking change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we need to explain what to configure in log.properties
separate lines of

javapackage=level

and what available levels are. In fact this should also be done better in the Trino docs btw

gateway-ha/src/main/java/io/trino/gateway/baseapp/BaseApp.java Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ActiveClusterMonitor.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsHttpMonitor.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/notifier/EmailNotifier.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/resource/TrinoResource.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/resource/TrinoResource.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/resource/TrinoResource.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueueLengthRoutingTable.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyServletImpl.java Outdated Show resolved Hide resolved
@ebyhr
Copy link
Member Author

ebyhr commented Mar 6, 2024

Rebased on main without any changes.

mosabua
mosabua approved these changes Mar 6, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor questions mostly .. but essentially good to go.

docs/installation.md Outdated
@@ -105,6 +105,14 @@ configuration YAML file.

Find more information in the [routing rules documentation](routing-rules.md).

### Configure logging

Path to `log.properties` needs to be set via `log.levels-file` JVM options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Path to `log.properties` needs to be set via `log.levels-file` JVM options
Path to `log.properties` must be set via `log.levels-file` JVM options

docs/installation.md Outdated
Path to `log.properties` needs to be set via `log.levels-file` JVM options
like `-Dlog.levels-file=etc/log.properties`.

Find more information in the [logging properties](https://trino.io/docs/current/admin/properties-logging.html)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Find more information in the [logging properties](https://trino.io/docs/current/admin/properties-logging.html)
Use the `log.*` properties from the [Trino logging properties documentation](https://trino.io/docs/current/admin/properties-logging.html) for further configuration.

probably .. but .. those have to also be set as -D parameters... need to explain that.

docs/installation.md Outdated
@@ -105,6 +105,14 @@ configuration YAML file.

Find more information in the [routing rules documentation](routing-rules.md).

### Configure logging

Path to `log.properties` needs to be set via `log.levels-file` JVM options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we need to explain what to configure in log.properties
separate lines of

javapackage=level

and what available levels are. In fact this should also be done better in the Trino docs btw

gateway-ha/pom.xml
@@ -205,7 +208,7 @@

<dependency>
<groupId>io.dropwizard</groupId>
<artifactId>dropwizard-logging</artifactId>
<artifactId>dropwizard-request-logging</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to get rid of this and just use jetty directly .. but for now it might have to stay

gateway-ha/pom.xml
@@ -235,6 +238,11 @@
<version>2.1.1</version>
</dependency>

<dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the suggested use from airlift/trino for the servlet api

@ebyhr ebyhr merged commit d22a32b into trinodb:main Mar 7, 2024
2 checks passed
@ebyhr ebyhr deleted the ebi/airlift-log branch March 7, 2024 01:07
@github-actions github-actions bot added this to the 7 milestone Mar 7, 2024
@mosabua mosabua mentioned this pull request Mar 18, 2024
Merged
@oneonestar oneonestar mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua approved these changes

@willmostly willmostly willmostly left review comments

@wendigo wendigo Awaiting requested review from wendigo

Assignees
No one assigned
Labels
cla-signed
Milestone
7
Development

Successfully merging this pull request may close these issues.

Use INFO log level by default in tests
3 participants
@ebyhr @mosabua @willmostly