You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Stove-hust (via GitHub)" <gi...@apache.org> on 2023/03/14 09:25:17 UTC
[GitHub] [spark] Stove-hust opened a new pull request, #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Stove-hust opened a new pull request, #40412:
URL: https://github.com/apache/spark/pull/40412
### What changes were proposed in this pull request?
Fixed a minor issue with diskBlockManager after push-based shuffle is enabled
### Why are the changes needed?
this bug will affect the efficiency of push based shuffle
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit test
--
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] Stove-hust commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1208740075
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
> Having said that, since the directory in this case is entirely managed by ESS currently, we can technically add it later as well - just makes it a bit less robust
ok, tks for your review
--
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] zhouyejoe commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "zhouyejoe (via GitHub)" <gi...@apache.org>.
zhouyejoe commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1471342162
Thanks for creating the PR. Will review ASAP @Stove-hust
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1200232145
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
(2) is an assumption which can break as things evolve or there are some corruption issues, etc.
Given we have already made the disk metadata op to list files, the additional validation is cheap
--
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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1550984254
> So to make sure I understand the issue here @Stove-hust - we have a container started on a node, and got immediately killed (pre-empted/etc) while it was in the process of creating the subdirs (started, but did not complete) ?
>
> If yes, very interesting corner case - will take a pass through the PR later this week, thanks for the contribution !
yep，that's the scenario you described
--
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] Stove-hust commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1198448075
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
It is very common to use multiple Executors for the same Application on the same machine, and as you say, the `listFiles` operation is almost inevitable, so I think it is necessary to think about what you are saying.
First of all, it would be possible to cache the list of subdirectories in memory after `listFiles`, and then check the existence of the subdirectories when they are created, but this would use some memory (albeit small, and we could manually set it to null after creating the directory)
I'm not sure what your concern is, but I can try to understand it, is it that there are dirty directories under the merge directory? Because that would cause some of the subdirectories not to be created. But I think this worry is not necessary, because under normal circumstances, due to permission related issues, there will be no dirty directories in the merge directory, so there is no problem to determine the number of subdirectories directly.
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199782358
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I am not sure I follow the comment above.
What I meant was, instead of simply checking for expected file count - check for the actual directories we expect to find there.
As pseudo-code, something like:
```
val dirs = Option(mergeDir.listFiles()).map(_.filter(_.isDirectory)).map(_.toSet)
if (!mergeDir.exists() || dirs.isEmpty ||
(0 until subDirsPerLocalDir).map(dirNum => new File(mergeDir, "%02x".format(dirNum))).exists(f => !dirs.get.contains(f)))
```
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199782358
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I am not sure I follow the comment above.
What I meant was, instead of simply checking for expected file count - check for the actual directories we expect to find there.
```
val dirs = Option(mergeDir.listFiles()).map(_.filter(_.isDirectory)).map(_.toSet)
if (!mergeDir.exists() || dirs.isEmpty ||
(0 until subDirsPerLocalDir).map(dirNum => new File(mergeDir, "%02x".format(dirNum))).exists(f => !dirs.get.contains(f)))
```
--
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] zhouyejoe commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "zhouyejoe (via GitHub)" <gi...@apache.org>.
zhouyejoe commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1478751685
@Stove-hust I think the changes will help resolve the issue described in the ticket. I am checking more about what could be causing to the race conditions where there are two Executor containers starting at almost the same time on the same node, and they will all enter this code section and try creating subdirs at the same time.
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1549125020
So to make sure I understand the issue here - we have a container started on a node, and got immediately killed (pre-empted/etc) - such that it was not able to complete creating `subDirsPerLocalDir` number of subdirectories (ended up create a few) ?
If yes, very interesting corner case - will take a pass through the PR later this week, thanks for the contribution !
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1198122448
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
Instead of only checking for the count, we can check if the files returned by listFiles match the expected filenames.
--
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] zhouyejoe commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "zhouyejoe (via GitHub)" <gi...@apache.org>.
zhouyejoe commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1179654420
##########
core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala:
##########
@@ -108,8 +108,8 @@ class DiskBlockManagerSuite extends SparkFunSuite {
assert(Utils.getConfiguredLocalDirs(testConf).map(
rootDir => new File(rootDir, DiskBlockManager.MERGE_DIRECTORY))
.filter(mergeDir => mergeDir.exists()).length === 2)
- // mergeDir0 will be skipped as it already exists
- assert(mergeDir0.list().length === 0)
+ // mergeDir0 can not be skipped evenif it already exists
Review Comment:
Nit: evenif ====> even if
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1615441438
Sorry for the delay on this PR, it slipping through my TODO list.
The change looks good to me, merging 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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1467724593
@zhouyejoe can you review this PR? i would be very grateful!
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199782358
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I am not sure I follow the comment above.
What I meant was, instead of simply checking for expected file count - check for the actual directories we expect to find there.
As pseudo-code, something like:
```
val dirs = Option(mergeDir.listFiles()).map(_.filter(_.isDirectory)).map(_.toSet)
if (!mergeDir.exists() || dirs.isEmpty || dirs.get.size != subDirsPerLocalDir ||
(0 until subDirsPerLocalDir).map(dirNum => new File(mergeDir, "%02x".format(dirNum))).exists(f => !dirs.get.contains(f)))
```
--
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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1537894454
@cloud-fan
--
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] Stove-hust commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1196157469
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
Sorry, I didn't understand what you said. Could you please describe it 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] mridulm commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1200246546
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
Having said that, since the directory in this case is entirely managed by ESS currently, we can technically add it later as well - just makes it a bit less robust
--
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 closed pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
URL: https://github.com/apache/spark/pull/40412
--
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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1547331364
@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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1550978743
>
yep
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1196128605
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
If we are calling listFiles anyway - why not validate they match the expected pattern ?
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
QQ: If we are calling `listFiles` anyway - why not validate they match the expected pattern ?
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199782358
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I am not sure I follow the comment above.
What I meant was, instead of simply checking for expected file count - check for the actual directories we expect to find there.
Something like:
```
val dirs = Option(mergeDir.listFiles()).map(_.filter(_.isDirectory)).map(_.toSet)
if (!mergeDir.exists() || dirs.isEmpty ||
(0 until subDirsPerLocalDir).map(dirNum => new File(mergeDir, "%02x".format(dirNum))).exists(f => !dirs.get.contains(f)))
```
--
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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1469223693
@mridulm Hello, can you recruit someone to help review this pr。I would appreciate for your help
--
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] Stove-hust commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1208739955
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
>
agree
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199782358
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I am not sure I follow the comment above.
What I meant was, instead of simply checking for expected file count - check for the actual directories we expect to find there.
As pseudo-code, something like:
```
val dirs = Option(mergeDir.listFiles()).map(_.filter(_.isDirectory)).map(_.toSet)
if (!mergeDir.exists() || dirs.isEmpty || dirs.get.size < subDirsPerLocalDir ||
(0 until subDirsPerLocalDir).map(dirNum => new File(mergeDir, "%02x".format(dirNum))).exists(f => !dirs.get.contains(f)))
```
--
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 #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1200232145
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
(2) is an assumption which can break as things evolve
--
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] Stove-hust commented on a diff in pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on code in PR #40412:
URL: https://github.com/apache/spark/pull/40412#discussion_r1199893005
##########
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala:
##########
@@ -273,7 +273,7 @@ private[spark] class DiskBlockManager(
Utils.getConfiguredLocalDirs(conf).foreach { rootDir =>
try {
val mergeDir = new File(rootDir, mergeDirName)
- if (!mergeDir.exists()) {
+ if (!mergeDir.exists() || mergeDir.listFiles().length < subDirsPerLocalDir) {
Review Comment:
I understand what you mean, but I still don't think it's necessary to check if the subdirectories match, based on the following two points
1. when creating a subdirectory, the file system ensures that it will not be created repeatedly (`if(!subDir.exists())`)
2. theoretically, mergeDir is managed directly by each App itself, and there will be no subdirectories that don't match our expectations
Therefore, determining the number of subdirectories is the same as matching each subdirectory.
--
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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1534022451
@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] Stove-hust commented on pull request #40412: [SPARK-42784] should still create subDir when the number of subDir in merge dir is less than conf
Posted by "Stove-hust (via GitHub)" <gi...@apache.org>.
Stove-hust commented on PR #40412:
URL: https://github.com/apache/spark/pull/40412#issuecomment-1478854808
@zhouyejoe I I kept the accident scene, should be able to help you。（In our clustered machine environment，11 HDD，creating 11 * 64 subdirectories would take longer to create）
23/02/21 10:44:09 YarnAllocator: Launching container container_184 on host hostA
23/02/21 10:44:21 YarnAllocator: Launching container container_185 on host hostA
23/02/21 10:44:21 YarnAllocator: Container container_184 on host: hostA was preempted.
--
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