Skip to content

MR: use encryption manager for input files#1532

Merged
rdblue merged 3 commits into
apache:masterfrom
chenjunjiedada:use-encryption-manager
Oct 1, 2020
Merged

MR: use encryption manager for input files#1532
rdblue merged 3 commits into
apache:masterfrom
chenjunjiedada:use-encryption-manager

Conversation

@chenjunjiedada

@chenjunjiedada chenjunjiedada commented Sep 30, 2020

Copy link
Copy Markdown
Contributor

This along with #1526 fixes #865.

chenjunjiedada
chenjunjiedada commented Sep 30, 2020
View reviewed changes
Comment thread mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergSplit.java Outdated
out.writeInt(data.length);
out.write(data);

byte[] ioData = SerializationUtil.serializeToBytes(io);

@chenjunjiedada chenjunjiedada Sep 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rdblue @rdsr , I saw your comments in #985 about remove customized serialization. Now the IcebergSplit implements org.apache.hadoop.mapred.InputSplit which extends writable. So I keep serialization implementation. Does this make sense to you?

@rdsr rdsr Oct 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with that.

@rdblue rdblue requested a review from rdsr September 30, 2020 20:24
rdblue
rdblue reviewed Sep 30, 2020
View reviewed changes
Comment thread mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergSplit.java Outdated
this.encryptionManager = SerializationUtil.deserializeFromBytes(encryptionManagerData);
}

public FileIO fileIO() {

@rdblue rdblue Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is named io in Table, so I don't think a longer name is needed here.

@rdblue

rdblue commented Sep 30, 2020

Copy link
Copy Markdown
Contributor

Looks good to me. Would be nice to rename that method to fit the convention used elsewhere.

@chenjunjiedada

chenjunjiedada commented Oct 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@rdblue , The HadoopFileIO contains SerailiableSupplier<Configuration>, so it should be serialized by SerializationUtil and then Writable, therefore satisfy the serialization requirement for MR framework. Does that make sense to you?

@chenjunjiedada chenjunjiedada force-pushed the use-encryption-manager branch 2 times, most recently from 0c94771 to b9b4553 Compare October 1, 2020 01:20
rdsr
rdsr approved these changes Oct 1, 2020
View reviewed changes

@rdsr rdsr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

Comment thread mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergSplit.java Outdated
out.writeInt(data.length);
out.write(data);

byte[] ioData = SerializationUtil.serializeToBytes(io);

@rdsr rdsr Oct 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with that.

@rdblue

rdblue commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

The HadoopFileIO contains SerailiableSupplier, so it should be serialized by SerializationUtil and then Writable, therefore satisfy the serialization requirement for MR framework.

Where is the HadoopFileIO being rebuilt with SerializableSupplier<Configuration>? In Spark, we have to check the FileIO and replace it with the one that references the SerializableSupplier.

@chenjunjiedada chenjunjiedada force-pushed the use-encryption-manager branch from ff6f4ee to 12b0906 Compare October 1, 2020 04:51
@chenjunjiedada

chenjunjiedada commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

@rdblue , Just added this. I missed that the configuration used to build SerialableSupplier<Configuration> also need to be serializable.

@rdblue

rdblue commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

Thanks for fixing it, @chenjunjiedada! I'll merge this.

@rdblue rdblue merged commit c7caa64 into apache:master Oct 1, 2020
@chenjunjiedada

chenjunjiedada commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for merging!

@rdblue rdblue mentioned this pull request Oct 23, 2020
Closed
@rdblue rdblue added this to the Java 0.10.0 Release milestone Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdblue rdblue rdblue left review comments
@rdsr rdsr rdsr approved these changes

Assignees

No one assigned

Labels

MR

Projects

None yet

Milestone

Java 0.10.0 Release

Development

Successfully merging this pull request may close these issues.

[InputFormat Followup] InputFiles should be created using FileIO and Encryption manager

3 participants

@chenjunjiedada @rdblue @rdsr