xkrogen commented on a change in pull request #34079:
URL: https://github.com/apache/spark/pull/34079#discussion_r715786353
##########
File path:
common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java
##########
@@ -199,11 +204,15 @@ protected void serviceInit(Configuration externalConf)
throws Exception {
_conf = new Configuration(externalConf);
URL confOverlayUrl = Thread.currentThread().getContextClassLoader()
.getResource(SHUFFLE_SERVICE_CONF_OVERLAY_RESOURCE_NAME);
+ String className = YarnShuffleService.class.getName();
if (confOverlayUrl != null) {
- logger.info("Initializing Spark YARN shuffle service with configuration
overlay from {}",
- confOverlayUrl);
_conf.addResource(confOverlayUrl);
}
+ String logsNamespace = _conf.get(SPARK_SHUFFLE_SERVICE_LOGS_NAMESPACE_KEY,
"");
+ String loggerName = logsNamespace.isEmpty() ? className : className + " "
+ logsNamespace;
+ logger = LoggerFactory.getLogger(loggerName);
+ logger.info("Initialized Spark YARN shuffle service with configuration
overlay from {}",
Review comment:
We need a different message if `confOverlayUrl` is null, right? Maybe
```
if (confOverlayUrl == null) {
logger.info("Initialized Spark YARN shuffle service using base
configuration");
} else {
logger.info(...); <- current logic here
}
```
##########
File path: docs/running-on-yarn.md
##########
@@ -783,6 +783,14 @@ The following extra configuration options are available
when the shuffle service
NodeManager.
</td>
</tr>
+<tr>
+ <td><code>spark.yarn.shuffle.service.logs.namespace</code></td>
+ <td><code>(not set)</code></td>
+ <td>
+ A namespace which will be appended to the class name when forming the
logger name to use for
+ emitting logs from the YARN shuffle service.
+ </td>
Review comment:
Once we finalize on how we add the namespace to the logger name (per the
comment I opened), we should provide an example here of what the final name
will look like
##########
File path:
common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java
##########
@@ -199,11 +204,15 @@ protected void serviceInit(Configuration externalConf)
throws Exception {
_conf = new Configuration(externalConf);
URL confOverlayUrl = Thread.currentThread().getContextClassLoader()
.getResource(SHUFFLE_SERVICE_CONF_OVERLAY_RESOURCE_NAME);
+ String className = YarnShuffleService.class.getName();
if (confOverlayUrl != null) {
- logger.info("Initializing Spark YARN shuffle service with configuration
overlay from {}",
- confOverlayUrl);
_conf.addResource(confOverlayUrl);
}
+ String logsNamespace = _conf.get(SPARK_SHUFFLE_SERVICE_LOGS_NAMESPACE_KEY,
"");
+ String loggerName = logsNamespace.isEmpty() ? className : className + " "
+ logsNamespace;
Review comment:
Maybe we can use something slightly more structured than a space
Right now assuming the namespace is "version311" we'll have something like:
```
INFO network.yarn.YarnShuffleService version311 Initialized Spark YARN
shuffle ...
```
Maybe this would look better:
```
INFO network.yarn.YarnShuffleService [version311] Initialized Spark YARN
shuffle ...
```
Or, since we're considering it part of the name and since frameworks may
assume the name looks like a class,
```
INFO network.yarn.YarnShuffleService.version311 Initialized Spark YARN
shuffle ...
```
(if we go with the last option because we think following the
class-name-like convention is valuable, then we should probably log a warning
if the namespace contains spaces)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
