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

ORC-1200: Extracting encryption setup logic from WriterImpl #1156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liujiawinds
Copy link
Contributor

@liujiawinds liujiawinds commented Jun 11, 2022

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.

@github-actions github-actions bot added the JAVA label Jun 11, 2022
@liujiawinds liujiawinds mentioned this pull request Jun 11, 2022
8 tasks
@dongjoon-hyun dongjoon-hyun modified the milestones: 1.8.0, 1.9.0 Jun 11, 2022
@dongjoon-hyun
Copy link
Member

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

It's unclear to me why this is requested by Apache Flink community. For example,

  1. Do you have some documentation or reference in Apache Flink community?
  2. Is this Apache ORC design issue?
  3. Is this request aligned with other encryption handling in Apache Flink community (like Apache Parquet Modular Encryption handling)?

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

Also, cc @omalley because he is the main author of Encryption Feature.

@liujiawinds
Copy link
Contributor Author

liujiawinds commented Jun 11, 2022

@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.

@dongjoon-hyun
Copy link
Member

Thank you for the details.

@liujiawinds
Copy link
Contributor Author

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

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 dongjoon-hyun removed this from the 1.9.0 milestone Jun 13, 2022
@dongjoon-hyun
Copy link
Member

After rethinking about this PR, I removed the milestone.

@liujiawinds
Copy link
Contributor Author

@dongjoon-hyun The PR description has been revised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants