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

Transfer output file compression option into ResultEventListener #966

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from

Conversation

danielfeismann
Copy link
Member

Resolves #965

@danielfeismann danielfeismann self-assigned this Sep 23, 2024
@danielfeismann danielfeismann added the code quality Code readability or efficiency is improved label Sep 23, 2024
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

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

Wait a minute... Didn't we already have this feature already?

ResultEntityCsvSink(
fileName.replace(".gz", ""),
new ResultEntityProcessor(resultClass),
fileName.endsWith(".gz"),
),

// compress files if necessary
if (compressOutputFiles)
Await.ready(zipAndDel(outfileName), 100.minutes)

Maybe we should also add this to documentation so it's not easily missed!

@danielfeismann danielfeismann marked this pull request as ready for review September 27, 2024 13:00
@danielfeismann danielfeismann marked this pull request as draft September 27, 2024 13:46
@danielfeismann danielfeismann marked this pull request as ready for review October 1, 2024 11:07
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! Just two comments from my side

Comment on lines +94 to +96
case _ =>
throw new ProcessResultEventException(
s"Invalid output file format for file $fileName provided. Currently only '.csv' or '.csv.gz' is supported!"
Copy link
Member

Choose a reason for hiding this comment

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

This case is never reached. My suggestion for the whole thing would be:

  • fileName.endsWith(".csv.gz") and compression = true => remove .gz
  • fileName.endsWith(".csv") => filename as-is
  • else => throw exception (with adapted message)

)
}

filePathFuture.flatMap { fileName =>
Copy link
Member

Choose a reason for hiding this comment

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

If you use map here, you don't need the Future { ... } below!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Code readability or efficiency is improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer output file compression option into ResultEventListener
2 participants