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/09/08 04:14:03 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
LuciferYang opened a new pull request, #37826:
URL: https://github.com/apache/spark/pull/37826
### What changes were proposed in this pull request?
There are 2 `initDB` in `DBProvider`
1. `DB initDB(DBBackend dbBackend, File dbFile, StoreVersion version, ObjectMapper mapper)`
2. `DB initDB(DBBackend dbBackend, File file)`
The first method is used in production code and and the second one only used by `ShuffleTestAccessor` for `YarnShuffleIntegrationSuite` as follows:
https://github.com/apache/spark/blob/0be27b6735de0edaccbc3c027b2ff9082a180c95/resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnShuffleIntegrationSuite.scala#L151-L170
This pr change to use the first method instead of second one.
### Why are the changes needed?
Code clean up.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass Github Actions
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1242725906
friendly ping @sunchao, do you know `MiniYARNCluster` can start with `YarnConfiguration.NM_RECOVERY_ENABLED = true`? I try set this to YarnConfig, and `MiniYARNCluster` start failed:
```
2022-09-10T11:44:42.1710230Z Cause: java.lang.ClassNotFoundException: org.apache.hadoop.shaded.org.iq80.leveldb.DBException
2022-09-10T11:44:42.1715234Z at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
2022-09-10T11:44:42.1719347Z at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
2022-09-10T11:44:42.1723090Z at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
2022-09-10T11:44:42.1726759Z at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
2022-09-10T11:44:42.1731028Z at org.apache.hadoop.yarn.server.nodemanager.NodeManager.initAndStartRecoveryStore(NodeManager.java:313)
2022-09-10T11:44:42.1735424Z at org.apache.hadoop.yarn.server.nodemanager.NodeManager.serviceInit(NodeManager.java:370)
2022-09-10T11:44:42.1740303Z at org.apache.hadoop.service.AbstractService.init(AbstractService.java:164)
2022-09-10T11:44:42.1745576Z at org.apache.hadoop.yarn.server.MiniYARNCluster$NodeManagerWrapper.serviceInit(MiniYARNCluster.java:597)
2022-09-10T11:44:42.1828858Z at org.apache.hadoop.service.AbstractService.init(AbstractService.java:164)
2022-09-10T11:44:42.1829712Z at org.apache.hadoop.service.CompositeService.serviceInit(CompositeService.java:109)
2022-09-10T11:44:42.1830633Z at org.apache.hadoop.yarn.server.MiniYARNCluster.serviceInit(MiniYARNCluster.java:327)
2022-09-10T11:44:42.1831431Z at org.apache.hadoop.service.AbstractService.init(AbstractService.java:164)
2022-09-10T11:44:42.1832279Z at org.apache.spark.deploy.yarn.BaseYarnClusterSuite.beforeAll(BaseYarnClusterSuite.scala:112)
```
I found `org.iq80.leveldb.*` relocated to `org.apache.hadoop.shaded.org.iq80.leveldb` in `NMLeveldbStateStoreService`, but it not shaded to hadoop-client-minicluster-3.3.4.jar.
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1240196417
cc @mridulm @tgravescs and @squito for further discussion, the previous thread is here https://github.com/apache/spark/pull/37648#discussion_r963271711
--
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 closed pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang closed pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
URL: https://github.com/apache/spark/pull/37826
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1250060672
> If that looks like a hassle then I would say we just ignore the test as one of those things that we can't unit test easily
Let me try `spark.testing.XXXX`
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1240239043
GA failed not related to current pr, need wait https://github.com/apache/spark/pull/37815
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967405666
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
Yes, I saw that `YarnShuffleServiceSuite ` called it. ~~But I'm a little curious, how does NodeManager set `RecoveryPath `? If not set, will LevelDB not be initialized? Let me investigate 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
[GitHub] [spark] LuciferYang commented on pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1243368045
@tgravescs I tried to add `yarnConfig.setBoolean(YarnConfiguration.NM_RECOVERY_ENABLED, true)` to make registeredExecFile not null and re-enable the test to verify execStateCopy's reload, but it failed due to the limitation of MiniYARNCluster itself. Do we have any other way to make the test of `execStateCopy reload check` work again?
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1242763731
https://github.com/apache/spark/blob/bf5103ae5bb938102850160048e2a1656e648244/resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnShuffleIntegrationSuite.scala#L46-L54
add `yarnConfig.setBoolean(YarnConfiguration.NM_RECOVERY_ENABLED, true)` to above `newYarnConfig` method, and test with hadoop-2.7
```
build/mvn clean install -pl resource-managers/yarn -Pyarn -Dtest=none -DwildcardSuites=org.apache.spark.deploy.yarn.YarnShuffleIntegrationWithLevelDBBackendSuite -Phadoop-2
```
then failed as follows:
```
YarnShuffleIntegrationWithLevelDBBackendSuite:
org.apache.spark.deploy.yarn.YarnShuffleIntegrationWithLevelDBBackendSuite *** ABORTED ***
java.lang.IllegalArgumentException: Cannot support recovery with an ephemeral server port. Check the setting of yarn.nodemanager.address
at org.apache.hadoop.yarn.server.nodemanager.containermanager.ContainerManagerImpl.serviceStart(ContainerManagerImpl.java:395)
at org.apache.hadoop.service.AbstractService.start(AbstractService.java:193)
at org.apache.hadoop.service.CompositeService.serviceStart(CompositeService.java:120)
at org.apache.hadoop.yarn.server.nodemanager.NodeManager.serviceStart(NodeManager.java:272)
at org.apache.hadoop.service.AbstractService.start(AbstractService.java:193)
at org.apache.hadoop.yarn.server.MiniYARNCluster$NodeManagerWrapper.serviceStart(MiniYARNCluster.java:560)
at org.apache.hadoop.service.AbstractService.start(AbstractService.java:193)
at org.apache.hadoop.service.CompositeService.serviceStart(CompositeService.java:120)
at org.apache.hadoop.yarn.server.MiniYARNCluster.serviceStart(MiniYARNCluster.java:278)
at org.apache.hadoop.service.AbstractService.start(AbstractService.java:193)
...
Run completed in 3 seconds, 992 milliseconds.
Total number of tests run: 0
Suites: completed 1, aborted 1
Tests: succeeded 0, failed 0, canceled 0, ignored 0, pending 0
*** 1 SUITE ABORTED ***
```
It seems that we need to use a static port to support testing recovery
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1242764058
So I think `assert(!ShuffleTestAccessor.reloadRegisteredExecutors(dbBackend, execStateCopy).isEmpty)` is and the relevant code is an unreachable code currently... @tgravescs @mridulm
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967405666
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
Yes, I saw that `YarnShuffleServiceSuite ` called it. But I'm a little curious, how does NodeManager set `RecoveryPath `? If not set, will LevelDB not be initialized? Let me investigate 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
[GitHub] [spark] LuciferYang commented on pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1250060909
> The only other way I can think of is to have recovery enabled when a test config is set (spark.testing.XXXX). If that looks like a hassle then I would say we just ignore the test as one of those things that we can't unit test easily
Sorry for missing this comment
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967386144
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
When I wanted to create some failed cases to compare differences before and after this pr, I found an interesting thing:
There are 3 cases are related to the code we discussed: `YarnShuffleIntegrationWithLevelDBBackendSuite`, `YarnShuffleAuthWithLevelDBBackendSuite`, `YarnShuffleAlternateNameConfigWithLevelDBBackendSuite`.
But when I add `println(s"registeredExecFile = $registeredExecFile")` after `val registeredExecFile = YarnTestAccessor.getRegisteredExecutorFile(shuffleService)(line 73 as follow code)`
https://github.com/apache/spark/blob/5a599dec507786139fb2ecb7ce1a44c83fd06b0d/resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnShuffleIntegrationSuite.scala#L69-L86
I found all 3 case print `registeredExecFile = null`, so the code we discussed was not actually executed due to registeredExecFile is always null ....
I also test these 3 cases use Spark 3.3, The problem also exists(registeredExecFile = null), I haven't found out which version of this code began to not execute, but this may be a very old bug.
@tgravescs @mridulm do you know when `YarnShuffleService` should call the `setRecoveryPath` method? It seems that these tests did not call the `setRecoveryPath` before serviceInit, so `registeredExecFile` is always null
--
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] sunchao commented on pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
sunchao commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1242764574
Thanks for the ping @LuciferYang , I'll take a look at the shading issue in Hadoop 3.3.4.
--
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] Yikun commented on pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
Yikun commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1240310375
GHCR recovered just do a retriggered
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967950567
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
Give a pr to fix this description: https://github.com/apache/spark/pull/37853
--
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] tgravescs commented on a diff in pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
tgravescs commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967392150
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
I don't remember in this particular case, its called from YarnShuffleServiceSuite, maybe it was never setup correctly, in which case maybe it doesn't matter if we change behavior
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
I don't remember in this particular case, its called from YarnShuffleServiceSuite, maybe it was never setup correctly, in which case maybe it doesn't matter and we aren't changing behavior
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967630119
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
@mridulm It seems that `LevelDB/RocksDB` diskstore is not always enabled when `YarnShuffleService` is used
https://github.com/apache/spark/blob/0996a1588feabea0aa3578bd551dcc1a1627eab7/docs/spark-standalone.md?plain=1#L321-L330
the description(`yarn always has this behavior enabled`) in `spark.shuffle.service.db.enabled` is incorrect, the behavior of YarnShuffleService is not controlled by this config
--
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 pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1243340146
I am unsure of this test - will rely on @tgravescs to comment better.
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967629206
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
from context of https://github.com/apache/spark/pull/19032, The enable or disable of LevelDB is related to `YarnConfiguration.NM_RECOVERY_ENABLED`
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1242764687
thanks @sunchao
--
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] tgravescs commented on pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #37826:
URL: https://github.com/apache/spark/pull/37826#issuecomment-1243778635
The only other way I can think of is to have recovery enabled when a test config is set (spark.testing.XXXX). If that looks like a hassle then I would say we just ignore the test as one of those things that we can't unit test easily
--
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] tgravescs commented on a diff in pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
tgravescs commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967137328
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
so as @mridulm said in the previous review this does add a bit of behavior change where we could hide a corruption that likely would have failed the test before. It would seem like that is a pretty rare case though and the test wasn't specifically trying to test that.
Honestly I can go either way on this change, I'm ok with it but at the same time don't think these extra functions are much maintenance unless you had other reasons for removing?
--
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 #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967405666
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
Yes, I saw that `YarnShuffleServiceSuite ` called it. But I'm a little curious. How does NodeManager set `YarnShuffleServiceSuite `? If not set, will LevelDB not be initialized? Let me investigate 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
[GitHub] [spark] LuciferYang commented on a diff in pull request #37826: [SPARK-40364][CORE] Use the unified `DBProvider#initDB ` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37826:
URL: https://github.com/apache/spark/pull/37826#discussion_r967405666
##########
common/network-common/src/main/java/org/apache/spark/network/util/LevelDBProvider.java:
##########
@@ -85,14 +84,6 @@ public static DB initLevelDB(File dbFile, StoreVersion version, ObjectMapper map
return tmpDb;
}
- @VisibleForTesting
Review Comment:
Yes, I saw that `YarnShuffleServiceSuite ` called it. But I'm a little curious, how does NodeManager set `YarnShuffleServiceSuite `? If not set, will LevelDB not be initialized? Let me investigate 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