You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "onebox-li (via GitHub)" <gi...@apache.org> on 2023/05/23 04:01:00 UTC
[GitHub] [incubator-celeborn] onebox-li opened a new pull request, #1505: [CELEBORN-599] Fix worker dirs get mount point
onebox-li opened a new pull request, #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505
### What changes were proposed in this pull request?
Currently `DeviceInfo#getMountPoint` match the longest string to determine which mount point a directory attached. May get wrong judges in some scenarios.
Assume mount points are /, /data,and dir is /data1. /data1 will attach to /data(should be /), and actually create shuffle dir in /.
### Why are the changes needed?
Improve usage, and avoid filling the wrong mount point
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
UT and cluster 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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#issuecomment-1558493347
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1505?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1505](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1505?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (41bf7f0) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/6e166662f1e27f21b50174e4990efd73c2fff28b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6e16666) will **decrease** coverage by `0.03%`.
> The diff coverage is `70.00%`.
```diff
@@ Coverage Diff @@
## main #1505 +/- ##
==========================================
- Coverage 45.27% 45.23% -0.03%
==========================================
Files 158 158
Lines 9743 9753 +10
Branches 959 966 +7
==========================================
+ Hits 4410 4411 +1
- Misses 5040 5044 +4
- Partials 293 298 +5
```
| [Impacted Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/celeborn/common/meta/DeviceInfo.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldGEvRGV2aWNlSW5mby5zY2FsYQ==) | `82.61% <70.00%> (-3.65%)` | :arrow_down: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1505/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#discussion_r1201532309
##########
common/src/main/scala/org/apache/celeborn/common/meta/DeviceInfo.scala:
##########
@@ -231,18 +231,22 @@ object DeviceInfo {
getMountPoint(f._1.getAbsolutePath, mountPointToDeviceInfo.keySet())
}.foreach {
case (mountPoint, dirs) =>
- val deviceInfo = mountPointToDeviceInfo.get(mountPoint)
- val diskInfo = new DiskInfo(
- mountPoint,
- dirs.map(_._1).toList,
- deviceInfo,
- conf)
- val (_, maxUsableSpace, threadCount, storageType) = dirs(0)
- diskInfo.configuredUsableSpace = maxUsableSpace
- diskInfo.threadCount = threadCount
- diskInfo.storageType = storageType
- deviceInfo.addDiskInfo(diskInfo)
- retDiskInfos.put(mountPoint, diskInfo)
+ if (mountPoint != "") {
Review Comment:
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#discussion_r1201524403
##########
common/src/test/scala/org/apache/celeborn/common/meta/DeviceInfoSuite.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.meta
+
+import java.util
+
+import org.junit.Assert.assertEquals
+
+import org.apache.celeborn.CelebornFunSuite
+
+class DeviceInfoSuite extends CelebornFunSuite {
+
+ test("test getMountPoint") {
+ val mountPoints = new util.HashSet[String]()
+ mountPoints.add("/")
+ mountPoints.add("/mnt/disk1")
+ mountPoints.add("/mnt/disk2")
+ mountPoints.add("/data")
+
+ assertEquals(DeviceInfo.getMountPoint("/mnt/disk1/data", mountPoints), "/mnt/disk1")
+ assertEquals(DeviceInfo.getMountPoint("/mnt/disk2/data", mountPoints), "/mnt/disk2")
+ assertEquals(DeviceInfo.getMountPoint("/data", mountPoints), "/data")
+ assertEquals(DeviceInfo.getMountPoint("/data/data", mountPoints), "/data")
+ assertEquals(DeviceInfo.getMountPoint("/data1/data", mountPoints), "/")
Review Comment:
thx, i will update later
##########
common/src/main/scala/org/apache/celeborn/common/meta/DeviceInfo.scala:
##########
@@ -231,18 +231,22 @@ object DeviceInfo {
getMountPoint(f._1.getAbsolutePath, mountPointToDeviceInfo.keySet())
}.foreach {
case (mountPoint, dirs) =>
- val deviceInfo = mountPointToDeviceInfo.get(mountPoint)
- val diskInfo = new DiskInfo(
- mountPoint,
- dirs.map(_._1).toList,
- deviceInfo,
- conf)
- val (_, maxUsableSpace, threadCount, storageType) = dirs(0)
- diskInfo.configuredUsableSpace = maxUsableSpace
- diskInfo.threadCount = threadCount
- diskInfo.storageType = storageType
- deviceInfo.addDiskInfo(diskInfo)
- retDiskInfos.put(mountPoint, diskInfo)
+ if (mountPoint != "") {
Review Comment:
thx, i will update later
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 merged pull request #1505: [CELEBORN-599] Consolidate calculation of mount point
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 merged PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#discussion_r1201491768
##########
common/src/main/scala/org/apache/celeborn/common/meta/DeviceInfo.scala:
##########
@@ -231,18 +231,22 @@ object DeviceInfo {
getMountPoint(f._1.getAbsolutePath, mountPointToDeviceInfo.keySet())
}.foreach {
case (mountPoint, dirs) =>
- val deviceInfo = mountPointToDeviceInfo.get(mountPoint)
- val diskInfo = new DiskInfo(
- mountPoint,
- dirs.map(_._1).toList,
- deviceInfo,
- conf)
- val (_, maxUsableSpace, threadCount, storageType) = dirs(0)
- diskInfo.configuredUsableSpace = maxUsableSpace
- diskInfo.threadCount = threadCount
- diskInfo.storageType = storageType
- deviceInfo.addDiskInfo(diskInfo)
- retDiskInfos.put(mountPoint, diskInfo)
+ if (mountPoint != "") {
Review Comment:
`mountPoint.nonEmpty?`
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1505: [CELEBORN-599] Consolidate calculation of mount point
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#issuecomment-1558585424
Thanks, merged to main for 0.3
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#discussion_r1201491672
##########
common/src/test/scala/org/apache/celeborn/common/meta/DeviceInfoSuite.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.meta
+
+import java.util
+
+import org.junit.Assert.assertEquals
+
+import org.apache.celeborn.CelebornFunSuite
+
+class DeviceInfoSuite extends CelebornFunSuite {
+
+ test("test getMountPoint") {
+ val mountPoints = new util.HashSet[String]()
+ mountPoints.add("/")
+ mountPoints.add("/mnt/disk1")
+ mountPoints.add("/mnt/disk2")
+ mountPoints.add("/data")
+
+ assertEquals(DeviceInfo.getMountPoint("/mnt/disk1/data", mountPoints), "/mnt/disk1")
+ assertEquals(DeviceInfo.getMountPoint("/mnt/disk2/data", mountPoints), "/mnt/disk2")
+ assertEquals(DeviceInfo.getMountPoint("/data", mountPoints), "/data")
+ assertEquals(DeviceInfo.getMountPoint("/data/data", mountPoints), "/data")
+ assertEquals(DeviceInfo.getMountPoint("/data1/data", mountPoints), "/")
Review Comment:
welcome to scala world
```suggestion
assert DeviceInfo.getMountPoint("/mnt/disk1/data", mountPoints) === "/mnt/disk1"
assert DeviceInfo.getMountPoint("/mnt/disk2/data", mountPoints) === "/mnt/disk2"
assert DeviceInfo.getMountPoint("/data", mountPoints) === "/data"
assert DeviceInfo.getMountPoint("/data/data", mountPoints) === "/data"
assert DeviceInfo.getMountPoint("/data1/data", mountPoints) === "/"
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1505: [CELEBORN-599] Fix worker dirs get mount point
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1505:
URL: https://github.com/apache/incubator-celeborn/pull/1505#discussion_r1201529803
##########
common/src/main/scala/org/apache/celeborn/common/meta/DeviceInfo.scala:
##########
@@ -231,18 +231,22 @@ object DeviceInfo {
getMountPoint(f._1.getAbsolutePath, mountPointToDeviceInfo.keySet())
}.foreach {
case (mountPoint, dirs) =>
- val deviceInfo = mountPointToDeviceInfo.get(mountPoint)
- val diskInfo = new DiskInfo(
- mountPoint,
- dirs.map(_._1).toList,
- deviceInfo,
- conf)
- val (_, maxUsableSpace, threadCount, storageType) = dirs(0)
- diskInfo.configuredUsableSpace = maxUsableSpace
- diskInfo.threadCount = threadCount
- diskInfo.storageType = storageType
- deviceInfo.addDiskInfo(diskInfo)
- retDiskInfos.put(mountPoint, diskInfo)
+ if (mountPoint != "") {
Review Comment:
BTW, would you mind updating line 230 to
```patch
- workingDirs.groupBy { f =>
- getMountPoint(f._1.getAbsolutePath, mountPointToDeviceInfo.keySet())
- }
+ workingDirs.groupBy { case (dir, _, _, _) =>
+ getMountPoint(dir.getCanonicalPath, mountPointToDeviceInfo.keySet())
+ }
```
we'd better use the canonical path as much as possible, in case the unexpected result
```
scala> new java.io.File("../b").getCanonicalPath
res7: String = /Users/b
scala> new java.io.File("../b").getAbsolutePath
res8: String = /Users/chengpan/../b
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org