[CELEBORN-2143] Create DiskFile sequentially based on createFileOrder#3594
[CELEBORN-2143] Create DiskFile sequentially based on createFileOrder#3594xy2953396112 wants to merge 1 commit intoapache:mainfrom
Conversation
|
hello @xy2953396112 maybe we could merge my patch and then rebase this patch ? |
|
I am reviewing this patch. |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Overall LGTM
I have left some suggestions
| retryCount += 1 | ||
| } | ||
| if (dfsStorageAvailable) { | ||
| logWarning("Failed to create localFileWriter", exception) |
| partitionDataWriterContext, | ||
| storageManager) | ||
| } else { | ||
| null |
There was a problem hiding this comment.
I suggest to add logDebug for these cases that return "null", sometimes I saw errors and I did not understand the reason
| partitionDataWriterContext, | ||
| storageManager) | ||
| } else { | ||
| null |
| partitionType: PartitionType, | ||
| partitionSplitEnabled: Boolean): (Flusher, DiskFileInfo, File) = { | ||
| val shuffleKey = Utils.makeShuffleKey(appId, shuffleId) | ||
| if (location.getStorageInfo.HDFSAvailable()) { |
There was a problem hiding this comment.
I think that the better solution looks like this:
Qbeast-io@460fc9b#diff-332230b33db740720657fd9c90e4f4eb0bce18b43f4749fddfe37463cc11a9b1R1125-R1128
we have to decouple the storage type to use from the "location"
|
@xy2953396112 in this PR I am adding a new suite of tests about TieredStorage maybe you can build on top of that and add tests that cover your changes and demonstrate the problem you are solving ? |
xy2953396112
force-pushed
the
CELEBORN-2143-2
branch
from
f4d9d63 to
3a817b0
Compare
2 participants
What changes were proposed in this pull request?
Create DiskFile sequentially based on createFileOrder.
Why are the changes needed?
Does this PR resolve a correctness bug?
NO
Does this PR introduce any user-facing change?
NO
How was this patch tested?
CI