Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per executor output v02 #337

Merged
merged 20 commits into from
Nov 7, 2024
Merged

Per executor output v02 #337

merged 20 commits into from
Nov 7, 2024

Conversation

EmileSonneveld
Copy link
Contributor

@EmileSonneveld EmileSonneveld commented Oct 18, 2024

Change for spatial and spatialTemporal cubes. A sync request that loads a single file remains unchanged.
Need to assure sample by feature is tested correctly
Double check if per-date tiffs are written from executor. See if when they have speculative execution, the problem is there too

@EmileSonneveld EmileSonneveld linked an issue Oct 29, 2024 that may be closed by this pull request
@EmileSonneveld EmileSonneveld requested a review from bossie October 29, 2024 13:11
Copy link
Collaborator

@bossie bossie left a comment

Choose a reason for hiding this comment

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

Basic algorithm looks fine, just some remarks to tidy up the code.

Comment on lines 280 to 281
val relativePath = Path.of(beforeOut).relativize(Path.of(absolutePath)).toString
val destinationPath = beforeOut + relativePath.substring(relativePath.indexOf("/") + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid string manipulation and call relativePath.getParent() (see above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does something different thatn getParent.
I added a comment:
// Remove the executorAttemptDirectory part from the path

@bossie
Copy link
Collaborator

bossie commented Oct 31, 2024

I'm not sure what happens if collect returns before competing executors have all finished; I suppose some files don't get cleaned up?

Java has a way to clean up temp files upon JVM exit that we could look into if needed (that would be a separate issue).

@bossie
Copy link
Collaborator

bossie commented Oct 31, 2024

More importantly, I also think that this only works if the batch job bucket is mounted because AFAICT you're treating every path as a file on disk.

@EmileSonneveld
Copy link
Contributor Author

collect should launch the executors and wait till they are finished.
I'll see if I can test without a mounted s3 bucket.
This trick is implemented to avoid an issue we only had with fusemounts, but I think it makes sense to keep it even with direct S3 access, as it avoids a racing condition.

@bossie
Copy link
Collaborator

bossie commented Oct 31, 2024

collect should launch the executors and wait till they are finished.

Yes, but speculative execution launches additional redundant executors and I should hope that collect does not wait for the slow ones.

@EmileSonneveld EmileSonneveld requested a review from bossie November 5, 2024 11:08
@EmileSonneveld
Copy link
Contributor Author

Speculative execution seems to kill remaining attempts when one succeeded: killed: another attempt succeeded
cleanUpExecutorAttemptDirectory handles their partial written files

@EmileSonneveld
Copy link
Contributor Author

@EmileSonneveld EmileSonneveld merged commit 8f51db3 into develop Nov 7, 2024
@EmileSonneveld EmileSonneveld deleted the per_executor_output_v02 branch November 21, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

separate_asset_per_band gives some empty tiffs
2 participants