orc icon indicating copy to clipboard operation
orc copied to clipboard

ORC-1200: Extracting encryption setup logic from `WriterImpl`

Open liujiawinds opened this issue 3 years ago • 7 comments

What changes were proposed in this pull request?

Extracting the encryption setup logic as a tool class.

Why are the changes needed?

Because of flink's ORC writer is based of stream, we must create a stream based PhysicalFsWriter before WriterImpl initial. WriterImpl Then, You can look into flink orc writer implementation from OrcBulkWriterFactory. Given the above, now I want to pass encryption settings to PhysicalFsWriter before WriterImpl initail, but the encryption setup at WriterImpl is so tightly coupled that the encryption variant cannot be obtained externally.

How was this patch tested?

It doesn't introduce new features and passed all test cases.

liujiawinds avatar Jun 11 '22 00:06 liujiawinds

Thank you for making a PR, @liujiawinds . I set the milestone 1.9.0 which is the version of main branch. For now, there is no plan of backporting this.

dongjoon-hyun avatar Jun 11 '22 02:06 dongjoon-hyun

@dongjoon-hyun

  1. Do you have some documentation or reference in Apache Flink community?

Yes, first we must create a stream based PhysicalFsWriter before WriterImpl initial. WriterImpl Then, You can look into flink orc writer from OrcBulkWriterFactory. Given the above, now I want to pass encryption settings to PhysicalFsWriter before WriterImpl initail, but the encryption setup at WriterImpl is so tightly coupled that the encryption variant cannot be obtained externally.

  1. Is this Apache ORC design issue?

Yes, I think it is.

  1. Is this request aligned with other encryption handling in Apache Flink community (like Apache Parquet Modular Encryption handling)?

Parquet has a similar class named FileEncryptionProperties. ParquetWriter

In general, branch-1.8 is already in feature freeze mode. I don't think this is required at v1.8.0 (currently).

It's ok to release in later versions. I will extract this part of code in flink.

liujiawinds avatar Jun 11 '22 05:06 liujiawinds

Thank you for the details.

dongjoon-hyun avatar Jun 11 '22 06:06 dongjoon-hyun

What should I do next for this pr? I don't quite understand the meaning of it being marked as Changes requested state. :)

liujiawinds avatar Jun 13 '22 01:06 liujiawinds

Instead of commenting, you are supposed to revise your PR description with your previous comment's content because only the PR title and description becomes a commit message.

What should I do next for this pr? I don't quite understand the meaning of it being marked as Changes requested state. :)

dongjoon-hyun avatar Jun 13 '22 06:06 dongjoon-hyun

After rethinking about this PR, I removed the milestone.

dongjoon-hyun avatar Jun 13 '22 06:06 dongjoon-hyun

@dongjoon-hyun The PR description has been revised.

liujiawinds avatar Jun 13 '22 07:06 liujiawinds