You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "eolivelli (via GitHub)" <gi...@apache.org> on 2026/02/25 15:46:50 UTC
[PR] [CELEBORN-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
eolivelli opened a new pull request, #3610:
URL: https://github.com/apache/celeborn/pull/3610
NOTE: this PR is stacked on top of https://github.com/apache/celeborn/pull/3608
Please consider only 756d25e49ef5f0321b90002d319b72924b9f4196
### What changes were proposed in this pull request?
Handle the eviction to a different location type.
### Why are the changes needed?
Because it may happen that a MEMORY file is to be evicted to another storage type (i.e. S3). This does not work.
Usually, as described in tests in #3608 when you have tiered storage, the primary partition type is usually not MEMORY, but it may happen that during a partition split the client decides to use MEMORY.
This patch fixes the problem on the worker side.
An alternative fix would be to change the behavior of the client, and simulate what the master does when offering slots.
Such a change would be more involved and it won't make the server side resilient to this scenario.
### Does this PR resolve a correctness bug?
No
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- New integration tests
- Manual testing on real k8s cluster with 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-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3610:
URL: https://github.com/apache/celeborn/pull/3610#discussion_r2871098316
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StoragePolicy.scala:
##########
@@ -94,6 +94,7 @@ class StoragePolicy(conf: CelebornConf, storageManager: StorageManager, source:
}
def tryCreateFileByType(storageInfoType: StorageInfo.Type): TierWriterBase = {
+ val overrideType = if (evict) storageInfoType else location.getStorageInfo.getType
Review Comment:
> @xy2953396112 is refactoring this code.
>
> I would prefer to keep this patch narrow and solve only the problem with eviction.
>
> It seems to me that having more that 2 storage types (memoery, hdd, s3...) still does not work well and it deserves a dedicated patch
Ok, make sense.
--
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-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3610: [CELEBORN-2270] Fix problem with eviction to tiered storage during partition split
URL: https://github.com/apache/celeborn/pull/3610
--
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-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3610:
URL: https://github.com/apache/celeborn/pull/3610#discussion_r2867017722
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StoragePolicy.scala:
##########
@@ -94,6 +94,7 @@ class StoragePolicy(conf: CelebornConf, storageManager: StorageManager, source:
}
def tryCreateFileByType(storageInfoType: StorageInfo.Type): TierWriterBase = {
+ val overrideType = if (evict) storageInfoType else location.getStorageInfo.getType
Review Comment:
Suppose location.getStorageInfo.getType is set to HDD and createFileOrder is [HDD, S3], if the local HDD fails to create a file, storageInfoType will switch to S3. See following code segment:
```
for (i <- tryCreateFileTypeIndex until maxSize) {
val storageStr = order.get(i)
val storageInfoType = StorageInfo.fromStrToType(storageStr)
val file = tryCreateFileByType(storageInfoType)
if (file != null) {
return file
}
}
```
However, when storageInfoType is set to S3, the method createDiskFile still uses location.getStorageInfo.getType as HDD, which seems incorrect. Therefore, StorageType seems should always be overrideType.
--
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-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3610:
URL: https://github.com/apache/celeborn/pull/3610#issuecomment-3983901695
Thanks. Merged 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-2270] Fix problem with eviction to tiered storage during partition split [celeborn]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #3610:
URL: https://github.com/apache/celeborn/pull/3610#discussion_r2871086665
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StoragePolicy.scala:
##########
@@ -94,6 +94,7 @@ class StoragePolicy(conf: CelebornConf, storageManager: StorageManager, source:
}
def tryCreateFileByType(storageInfoType: StorageInfo.Type): TierWriterBase = {
+ val overrideType = if (evict) storageInfoType else location.getStorageInfo.getType
Review Comment:
@xy2953396112 is refactoring this code.
I would prefer to keep this patch narrow and solve only the problem with eviction.
It seems to me that having more that 2 storage types (memoery, hdd, s3...) still does not work well and it deserves a dedicated patch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org