You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "psavalle (via GitHub)" <gi...@apache.org> on 2026/08/31 08:17:35 UTC
[PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
psavalle opened a new pull request, #58429:
URL: https://github.com/apache/spark/pull/58429
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'common/utils/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This is an attempt at [SPARK-58837](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-58837?filter=reportedbyme). We introduce per-module Python loggers for the Spark Connect Python client, so that logging levels can be controlled on a per-module basis.
For example, a client can now do:
```py
import logging
logging.getLogger("pyspark.sql.connect.client.retries").setLevel(logging.DEBUG)
```
to log retries.
### Why are the changes needed?
Currently, the Python Connect client logs everything using the `pyspark.sql.connect.logging` root logger, which is gated on the `SPARK_CONNECT_LOG_LEVEL` environment variable, so we cannot control logging on a fine-grained fashion.
For example, when trying to connect to a Connect server that does not respond, the client will silently retry and backoff, which can be confusing to users. Enabling logging of gRPC UNAVAILABLE retries would be useful, but is only possible by setting `SPARK_CONNECT_LOG_LEVEL=DEBUG`, which then produces way too much log volume.
### Does this PR introduce _any_ user-facing change?
Users can now control logging levels on a per-module level.
### How was this patch tested?
Unit tests
### Was this patch authored or co-authored using generative AI tooling?
`Generated-by: codex-cli 0.147.0`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "psavalle (via GitHub)" <gi...@apache.org>.
psavalle commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5625805656
`PySparkLogger` seems a do a couple things that do not seem desirable:
- [It pins all loggers at WARN](https://github.com/apache/spark/blob/master/python/pyspark/logger/logger.py#L144), which would prevent us from controlling logger levels hierarchically. Probably hard to change now without breaking backward compatibility?
- [It always attaches a handler](https://github.com/apache/spark/blob/1b8ef5fa621e4959063626ecc1ad328206c0d61f/python/pyspark/logger/logger.py#L156) -- so similar issue: if we have a hierarchy of loggers, a message would get logged N times, once at each level.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5625055568
Overall I think the direction is good. Let's discuss some details - why we give up on `PySparkLogger`? What's the advantage to use the raw logger instead of relying on that wrapper?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5765866866
My thought around this is how to avoid having duplicated code. We can add an optional `level` arg to `PySparkLogger` which defaults to `WARN`, so the existing behavior is kept. For connect loggers, it can inherit `PySparkLogger` or even just use it with a different `level` arg. The whole goal is to have the shared logic in one file instead of two.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5627169678
> [It pins all loggers at WARN](https://github.com/apache/spark/blob/master/python/pyspark/logger/logger.py#L144), which would prevent us from controlling logger levels hierarchically. Probably hard to change now without breaking backward compatibility?
It does not "pin" loggers at `WARN`, it initializes them at `WARN`. You can easily change them with `setLevel`.
> [It always attaches a handler](https://github.com/apache/spark/blob/1b8ef5fa621e4959063626ecc1ad328206c0d61f/python/pyspark/logger/logger.py#L156) -- so similar issue: if we have a hierarchy of loggers, a message would get logged N times, once at each level.
I think this behavior is just wrong. We should not by default add a handler for each logger - we should add the handler on root logger. The only place we used this in our code base, we set `logger.propagate = False`.
I think it's safe to change the current behavior - it's a relatively new feature and it's for logging. We should make the behavior reasonable. We don't need to backport it but we can fix it in `branch-4.x` and `master`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5485265207
I quite like this.. @gaogaotiantian would you mind taking a quick look when you're available?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58837][CONNECT] Use per-module Python loggers [spark]
Posted by "psavalle (via GitHub)" <gi...@apache.org>.
psavalle commented on PR #58429:
URL: https://github.com/apache/spark/pull/58429#issuecomment-5758754197
> It does not "pin" loggers at WARN, it initializes them at WARN. You can easily change them with setLevel.
We can change the level on individual loggers, but if all loggers are initialized at `WARN` instead of `NOTSET`, then we lose the ability to control log levels hierarchically -- e.g., to set the level for
`pyspark.sql.connect.client.core`, we would need to manually set the level for all of the child loggers.
I can take another stab at this so we use `PySparkLogger` instead, with a single handler and level set on the root logger, if we're fine changing the behavior on the levels as well?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org