You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/07/03 01:32:42 UTC
[GitHub] [spark] panbingkun opened a new pull request, #37058: [SPARK-39661][CORE][SHUFFLE][SQL] Avoid creating unnecessary SLF4J Logger
panbingkun opened a new pull request, #37058:
URL: https://github.com/apache/spark/pull/37058
### What changes were proposed in this pull request?
In java class, convert logger to static
In scala class, with Logging
The pr follow up: https://issues.apache.org/jira/browse/SPARK-39538 https://github.com/apache/spark/pull/36930
### Why are the changes needed?
To improve performance.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UT.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915851705
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/RowBasedKeyValueBatch.java:
##########
@@ -48,7 +48,7 @@
*
*/
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(RowBasedKeyValueBatch.class);
+ protected static final Logger logger = LoggerFactory.getLogger(RowBasedKeyValueBatch.class);
Review Comment:
this change is ok
--
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
[GitHub] [spark] srowen commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1175050327
Hm, tests timed out. Can you retrigger? I'm sure it's not related, but good to see tests pass to be sure.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #37058: [SPARK-39661][CORE][SHUFFLE][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1173011007
Can one of the admins verify this patch?
--
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
[GitHub] [spark] srowen commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915848934
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
Whoa, hm, I wonder how - some kind of locking issue now that the logger is shared?
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915896704
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
I wonder why this is not a Scala compile time error
--
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
[GitHub] [spark] LuciferYang commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1176314535
@panbingkun test still timeout
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915841186
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
Please don't merge this, I tested the timeout case locally use follow command:
```
build/sbt "core/testOnly *BlockManagerBasicStrategyReplicationSuite"
```
the test is really hang
<img width="1195" alt="image" src="https://user-images.githubusercontent.com/1475305/177778662-e3051024-038e-4ea2-84a9-d8c2bdc65a66.png">
and I test this case in master, all passed.
--
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
[GitHub] [spark] srowen closed pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
URL: https://github.com/apache/spark/pull/37058
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915841186
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
@srowen Please don't merge this, I tested the timeout case locally use follow command:
```
build/sbt "core/testOnly *BlockManagerBasicStrategyReplicationSuite"
```
the test is really hang
<img width="1195" alt="image" src="https://user-images.githubusercontent.com/1475305/177778662-e3051024-038e-4ea2-84a9-d8c2bdc65a66.png">
and I test this case in master, all passed.
--
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
[GitHub] [spark] srowen commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1177829935
Ah OK, ignore my suggestion to change BlockStoreClient. I don't quite get why it compiled either. but I think an logger in the instance might be correct there.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r914930748
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
hmm... If `static final` is used, I think the variable name should be capitalized.
--
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
[GitHub] [spark] srowen commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1178981149
Merged to 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
[GitHub] [spark] srowen commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1174493537
Same in `BlockStoreClient` - can you change that too?
--
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
[GitHub] [spark] panbingkun commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1175082503
> Hm, tests timed out. Can you retrigger? I'm sure it's not related, but good to see tests pass to be sure.
okay
--
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
[GitHub] [spark] mridulm commented on a diff in pull request #37058: [SPARK-39661][CORE][SHUFFLE][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r912638319
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala:
##########
@@ -56,7 +56,7 @@ private[spark] class NettyBlockTransferService(
_port: Int,
numCores: Int,
driverEndPointRef: RpcEndpointRef = null)
- extends BlockTransferService {
+ extends BlockTransferService with Logging {
Review Comment:
Given the `logger` exposed via `BlockStoreClient`, why use `Logging` here ?
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915892176
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
```
22/07/07 21:30:55.200 shuffle-client-39-1 WARN TransportChannelHandler: Exception in connection from localhost/127.0.0.1:52433
java.lang.IllegalAccessError: tried to access field org.apache.spark.network.shuffle.BlockStoreClient.logger from class org.apache.spark.network.netty.NettyBlockTransferService$$anon$3
at org.apache.spark.network.netty.NettyBlockTransferService$$anon$3.onSuccess(NettyBlockTransferService.scala:185)
at org.apache.spark.network.client.TransportResponseHandler.handle(TransportResponseHandler.java:196)
at org.apache.spark.network.server.TransportChannelHandler.channelRead0(TransportChannelHandler.java:142)
at org.apache.spark.network.server.TransportChannelHandler.channelRead0(TransportChannelHandler.java:53)
at io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:99)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.timeout.IdleStateHandler.channelRead(IdleStateHandler.java:286)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at org.apache.spark.network.util.TransportFrameDecoder.channelRead(TransportFrameDecoder.java:102)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:722)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:658)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:584)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:496)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:995)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.lang.Thread.run(Thread.java:750)
```
@srowen @panbingkun from above message, the `logger` in `BlockStoreClient` should be `public`
--
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
[GitHub] [spark] srowen commented on a diff in pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r915037435
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -42,7 +42,7 @@
* or external service.
*/
public abstract class BlockStoreClient implements Closeable {
- protected final Logger logger = LoggerFactory.getLogger(this.getClass());
+ protected static final Logger logger = LoggerFactory.getLogger(BlockStoreClient.class);
Review Comment:
Yeah go ahead and make it LOGGER and let's try tests one more time. I'm sure it's not related so would probably merge anyway.
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #37058: [SPARK-39661][CORE][SHUFFLE][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #37058:
URL: https://github.com/apache/spark/pull/37058#discussion_r912417783
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala:
##########
@@ -56,7 +56,7 @@ private[spark] class NettyBlockTransferService(
_port: Int,
numCores: Int,
driverEndPointRef: RpcEndpointRef = null)
- extends BlockTransferService {
+ extends BlockTransferService with Logging {
Review Comment:
"with Logging" is more consistent and natural here.
--
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
[GitHub] [spark] panbingkun commented on pull request #37058: [SPARK-39661][CORE][SHUFFLE][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1173398873
> Change from instance to static variable makes sense. You can revert changes to `NettyBlockTransferService`.
Reason：Because nettyblocktransferservice is a singleton in every executor ？
Ok，i will revert nettyblocktransferservice.
--
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
[GitHub] [spark] panbingkun commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1174626719
> Same in `BlockStoreClient` - can you change that too?
Updated.
--
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
[GitHub] [spark] panbingkun commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1178418051
> Ah OK, ignore my suggestion to change BlockStoreClient. I don't quite get why it compiled either. but I think an logger in the instance might be correct there.
BlockStoreClient's child class - NettyBlockTransferService, only be instantiated once in SparkEnv. code:
https://github.com/panbingkun/spark/blob/master/core/src/main/scala/org/apache/spark/SparkEnv.scala#L365-L367
so A JVM has only one NettyBlockTransferService object, change BlockStoreClient may not have any effect.
--
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
[GitHub] [spark] panbingkun commented on pull request #37058: [SPARK-39661][SQL] Avoid creating unnecessary SLF4J Logger
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #37058:
URL: https://github.com/apache/spark/pull/37058#issuecomment-1178418498
I will revert it!
--
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