Skip to content

[CELEBORN-2143] Create DiskFile sequentially based on createFileOrder#3594

Open
xy2953396112 wants to merge 1 commit intoapache:mainfrom
xy2953396112:CELEBORN-2143-2
Open

[CELEBORN-2143] Create DiskFile sequentially based on createFileOrder#3594
xy2953396112 wants to merge 1 commit intoapache:mainfrom
xy2953396112:CELEBORN-2143-2

Conversation

@xy2953396112
Copy link
Contributor

What changes were proposed in this pull request?

Create DiskFile sequentially based on createFileOrder.

Why are the changes needed?

  1. If the PartitionLocation specifies a StorageType (such as HDFS), DfsTierWriter should be created based on that StorageType.
  2. Optimize the storage strategy to create the logic for DiskFile.

Does this PR resolve a correctness bug?

NO

Does this PR introduce any user-facing change?

NO

How was this patch tested?

CI

@eolivelli
Copy link
Contributor

hello @xy2953396112
in my PR #3592 I am adding some integration test to validate that S3 works

maybe we could merge my patch and then rebase this patch ?

@eolivelli
Copy link
Contributor

I am reviewing this patch.
Can you please solve the conflicts ?

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM

I have left some suggestions

retryCount += 1
}
if (dfsStorageAvailable) {
logWarning("Failed to create localFileWriter", exception)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please report shuffleId

partitionDataWriterContext,
storageManager)
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here and below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants