You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "xy2953396112 (via GitHub)" <gi...@apache.org> on 2025/12/08 09:10:04 UTC
[PR] [CELEBORN-2235] Avoiding regular expressions for DiskFileInfo storage… [celeborn]
xy2953396112 opened a new pull request, #3562:
URL: https://github.com/apache/celeborn/pull/3562
… type determination
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Avoiding regular expressions for DiskFileInfo storage type determination
### Why are the changes needed?
Avoiding regular expressions for DiskFileInfo storage type determination
### Does this PR resolve a correctness bug?
NO
### Does this PR introduce _any_ user-facing change?
NO
### How was this patch tested?
CI
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645220334
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -114,12 +114,41 @@ object PbSerDeUtils {
fileMeta.setIsWriterClosed(true)
fileMeta
}
- new DiskFileInfo(
- userIdentifier,
- pbFileInfo.getPartitionSplitEnabled,
- meta,
- pbFileInfo.getFilePath,
- pbFileInfo.getBytesFlushed)
+ if (pbFileInfo.getStorageType != 0) {
Review Comment:
Could you simplify this implementation for better readability like following?
```
var storageType: StorageInfo.Type = null
if (pbFileInfo.getStorageType == 0) {
if (Utils.isHdfsPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.HDFS
} else if (Utils.isOssPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.OSS
} else if (Utils.isS3Path(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.S3
}
} else {
storageType = StorageInfo.typesMap.get(pbFileInfo.getStorageType)
}
new DiskFileInfo(userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
storageType,
pbFileInfo.getBytesFlushed)
```
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645220334
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -114,12 +114,41 @@ object PbSerDeUtils {
fileMeta.setIsWriterClosed(true)
fileMeta
}
- new DiskFileInfo(
- userIdentifier,
- pbFileInfo.getPartitionSplitEnabled,
- meta,
- pbFileInfo.getFilePath,
- pbFileInfo.getBytesFlushed)
+ if (pbFileInfo.getStorageType != 0) {
Review Comment:
Could you simplify this implementation for better readability like following?
```
var storageType: StorageInfo.Type = null
if (pbFileInfo.getStorageType == 0) {
if (Utils.isHdfsPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.HDFS
} else if (Utils.isOssPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.OSS
} else if (Utils.isS3Path(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.S3
}
} else {
storageType = StorageInfo.typesMap.get(pbFileInfo.getStorageType)
}
if (storageType == null) {
new DiskFileInfo(userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
pbFileInfo.getBytesFlushed)
} else {
new DiskFileInfo(userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
storageType,
pbFileInfo.getBytesFlushed)
}
```
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645650108
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -114,12 +114,41 @@ object PbSerDeUtils {
fileMeta.setIsWriterClosed(true)
fileMeta
}
- new DiskFileInfo(
- userIdentifier,
- pbFileInfo.getPartitionSplitEnabled,
- meta,
- pbFileInfo.getFilePath,
- pbFileInfo.getBytesFlushed)
+ if (pbFileInfo.getStorageType != 0) {
Review Comment:
thanks, I have updated the code.
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#issuecomment-3690790897
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3562?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 67.15%. Comparing base ([`ffff5bb`](https://app.codecov.io/gh/apache/celeborn/commit/ffff5bb94efd7c3a2c6c51aebccfd3c11b5d5198?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`508cdea`](https://app.codecov.io/gh/apache/celeborn/commit/508cdea18169d7e5cb00417163f7c2d016631b5e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 16 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3562 +/- ##
==========================================
+ Coverage 67.05% 67.15% +0.10%
==========================================
Files 357 357
Lines 21779 21860 +81
Branches 1930 1943 +13
==========================================
+ Hits 14602 14677 +75
- Misses 6160 6164 +4
- Partials 1017 1019 +2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/3562?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2617213648
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
Good point. Can we modify it to the following code?
```
public` boolean isHdfs() {
return storageType == StorageInfo.Type.HDFS || Utils.isHdfsPath(filePath);
}
```
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
Good point. Can we modify it to the following code?
```
public boolean isHdfs() {
return storageType == StorageInfo.Type.HDFS || Utils.isHdfsPath(filePath);
}
```
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645220334
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -114,12 +114,41 @@ object PbSerDeUtils {
fileMeta.setIsWriterClosed(true)
fileMeta
}
- new DiskFileInfo(
- userIdentifier,
- pbFileInfo.getPartitionSplitEnabled,
- meta,
- pbFileInfo.getFilePath,
- pbFileInfo.getBytesFlushed)
+ if (pbFileInfo.getStorageType != 0) {
Review Comment:
Could you simplify this implementation for better readability like following?
```
var storageType: StorageInfo.Type = null
if (pbFileInfo.getStorageType == 0) {
if (Utils.isHdfsPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.HDFS
} else if (Utils.isOssPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.OSS
} else if (Utils.isS3Path(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.S3
}
} else {
storageType = StorageInfo.typesMap.get(pbFileInfo.getStorageType)
}
if (storageType != null) {
new DiskFileInfo(
userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
storageType,
pbFileInfo.getBytesFlushed)
} else {
new DiskFileInfo(
userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
pbFileInfo.getBytesFlushed)
}
```
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645195230
##########
common/src/test/scala/org/apache/celeborn/common/util/PbSerDeUtilsTest.scala:
##########
@@ -51,6 +51,9 @@ class PbSerDeUtilsTest extends CelebornFunSuite {
val file1 = new File("/mnt/disk/1")
val file2 = new File("/mnt/disk/2")
+ val file3 = "hdfs://xxx:9000/xxxx/xx-xx/x-x-x"
+ val file4 = "oss://xxxx/xx-xx/x-x-x"
+ val file5 = "s3a://xxxx/xx-xx/x-x-x"
Review Comment:
Why use `s3a` to test S3 filesytem?
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 closed pull request #3562: [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination
URL: https://github.com/apache/celeborn/pull/3562
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #3562: [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination
URL: https://github.com/apache/celeborn/pull/3562
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#issuecomment-3705189266
Thanks, merge to main(v0.7.0)
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 closed pull request #3562: [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination
URL: https://github.com/apache/celeborn/pull/3562
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2617213648
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
Good point. Can we modify it to the following code?
`public boolean isHdfs() {
return storageType == StorageInfo.Type.HDFS || Utils.isHdfsPath(filePath);
}
`
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "ErikFang (via GitHub)" <gi...@apache.org>.
ErikFang commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2619022352
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
> Good point. Can we modify it to the following code?
>
> ```
> public boolean isHdfs() {
> return storageType == StorageInfo.Type.HDFS || Utils.isHdfsPath(filePath);
> }
> ```
I don't think this is a good idea, let's add storage type into serialized data instead
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645220334
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -114,12 +114,41 @@ object PbSerDeUtils {
fileMeta.setIsWriterClosed(true)
fileMeta
}
- new DiskFileInfo(
- userIdentifier,
- pbFileInfo.getPartitionSplitEnabled,
- meta,
- pbFileInfo.getFilePath,
- pbFileInfo.getBytesFlushed)
+ if (pbFileInfo.getStorageType != 0) {
Review Comment:
Could you simplify this implementation for better readability like following?
```
var storageType: StorageInfo.Type = null
if (pbFileInfo.getStorageType == 0) {
if (Utils.isHdfsPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.HDFS
} else if (Utils.isOssPath(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.OSS
} else if (Utils.isS3Path(pbFileInfo.getFilePath)) {
storageType = StorageInfo.Type.S3
}
} else {
storageType = StorageInfo.typesMap.get(pbFileInfo.getStorageType)
}
if (storageType != null) {
new DiskFileInfo(userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
storageType,
pbFileInfo.getBytesFlushed)
} else {
new DiskFileInfo(userIdentifier,
pbFileInfo.getPartitionSplitEnabled,
meta,
pbFileInfo.getFilePath,
pbFileInfo.getBytesFlushed)
}
```
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645195230
##########
common/src/test/scala/org/apache/celeborn/common/util/PbSerDeUtilsTest.scala:
##########
@@ -51,6 +51,9 @@ class PbSerDeUtilsTest extends CelebornFunSuite {
val file1 = new File("/mnt/disk/1")
val file2 = new File("/mnt/disk/2")
+ val file3 = "hdfs://xxx:9000/xxxx/xx-xx/x-x-x"
+ val file4 = "oss://xxxx/xx-xx/x-x-x"
+ val file5 = "s3a://xxxx/xx-xx/x-x-x"
Review Comment:
Why use `s3a` to test S3 filesytem?
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 closed pull request #3562: [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination
URL: https://github.com/apache/celeborn/pull/3562
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645233705
##########
common/src/test/scala/org/apache/celeborn/common/util/PbSerDeUtilsTest.scala:
##########
@@ -51,6 +51,9 @@ class PbSerDeUtilsTest extends CelebornFunSuite {
val file1 = new File("/mnt/disk/1")
val file2 = new File("/mnt/disk/2")
+ val file3 = "hdfs://xxx:9000/xxxx/xx-xx/x-x-x"
+ val file4 = "oss://xxxx/xx-xx/x-x-x"
+ val file5 = "s3a://xxxx/xx-xx/x-x-x"
Review Comment:
Please add the test case for the prefix `s3://`.
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2617043647
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
The current implementation of DiskFileInfo does not serialize the storage type information. As a result, when a worker recovers from a graceful shutdown, the storage type in DiskFileInfo defaults to StorageInfo.Type.HDD. This behavior can cause the method isDFS to return incorrect results due to the default assumption of the storage type as HDD. Maybe we should serialize storage type info first.
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2617213648
##########
common/src/main/java/org/apache/celeborn/common/meta/DiskFileInfo.java:
##########
@@ -150,19 +150,21 @@ public void setMountPoint(String mountPoint) {
}
public boolean isHdfs() {
- return Utils.isHdfsPath(filePath);
+ return storageType == StorageInfo.Type.HDFS;
Review Comment:
Good point. Can we modify it to the following code?
`
public boolean isHdfs() {
return storageType == StorageInfo.Type.HDFS || Utils.isHdfsPath(filePath);
}
`
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#issuecomment-3655708234
@RexXiong @ErikFang Thanks~, PTAL.
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#issuecomment-3677835500
> @RexXiong @ErikFang Thanks~, PTAL.
Agree with serialize storage type first, but It's also important to note that the storageType was not serialized before, which could lead to compatibility issues if used directly. We need think about this situation.
--
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
Re: [PR] [CELEBORN-2236] Avoiding regular expressions for DiskFileInfo storage type determination [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3562:
URL: https://github.com/apache/celeborn/pull/3562#discussion_r2645650674
##########
common/src/test/scala/org/apache/celeborn/common/util/PbSerDeUtilsTest.scala:
##########
@@ -51,6 +51,9 @@ class PbSerDeUtilsTest extends CelebornFunSuite {
val file1 = new File("/mnt/disk/1")
val file2 = new File("/mnt/disk/2")
+ val file3 = "hdfs://xxx:9000/xxxx/xx-xx/x-x-x"
+ val file4 = "oss://xxxx/xx-xx/x-x-x"
+ val file5 = "s3a://xxxx/xx-xx/x-x-x"
Review Comment:
thanks, I have added the test case.
--
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